Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Charter Africa Design Update #674

Merged
merged 23 commits into from
Feb 29, 2024
Merged

Charter Africa Design Update #674

merged 23 commits into from
Feb 29, 2024

Conversation

kelvinkipruto
Copy link
Contributor

@kelvinkipruto kelvinkipruto commented Feb 13, 2024

Description

This PR makes updates to the Datasets and Contributors pages as per the new designs here and here.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Screenshots

Datasets Page
image
image

Contributors Page
image
image

image

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@kelvinkipruto kelvinkipruto marked this pull request as ready for review February 16, 2024 13:41
@kilemensi
Copy link
Member

Didn't we use to have dev urls to see how the PR looks live @kelvinkipruto?

@kelvinkipruto
Copy link
Contributor Author

@kilemensi That was only for codeforafrica. It does not work for any of the other apps.

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽


  1. With these additional API calls, are we still within the limits?
  2. Has the dev DB been setup correctly so that if this PR has been merged, we can ask for review on dev site before deploying to prod?

@kilemensi
Copy link
Member

@koechkevin

Copy link
Contributor

@koechkevin koechkevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kelvinkipruto I have run the script locally and did not see the updated changes on contributor.

Can we catch up on this?

@kilemensi
Copy link
Member

@kelvinkipruto I have run the script locally and did not see the updated changes on contributor.

Can we catch up on this?

Is this issue resolve @koechkevin / @kelvinkipruto ?

Copy link
Contributor

@koechkevin koechkevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kelvinkipruto implementation looks good, however I notice 2 things.

  1. The token will need to be updated with a token with sufficient organisation scope permission.
  2. Looks like with payload version 2, setting limit to 0 on payload.find no longer gets all the collection items but rather 10 and not all entries will get updated when the cron runs.

@kilemensi
Copy link
Member

2. Looks like with payload version 2, setting limit to 0 on `payload.find` no longer gets all the collection items but rather 10 and not all entries will get updated when the cron runs.

Are you talking about disabling pagination @koechkevin ?

@koechkevin
Copy link
Contributor

2. Looks like with payload version 2, setting limit to 0 on `payload.find` no longer gets all the collection items but rather 10 and not all entries will get updated when the cron runs.

Are you talking about disabling pagination @koechkevin ?

Yes @kilemensi

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

--

  1. Rename the icons to follow the current naming scheme. It allows us to see the size, color, etc., of the icon at a glance without opening it.
  2. I see some of the icons have different size/viewBox e.g. discord is 25/"0 0 25 25". If an icon size is not 24 (the default in MUI), you must set viewBox prop on the SVGIcon component so that MUI scales it accordingly.

@koechkevin
Copy link
Contributor

@kilemensi an update on this.

Using github graphql to fetch a user causes the script to exceed github's rate limits.
Thinking out a workaround for this is bulkfetch all the users in a single graphql query together with aliases which works upto around 100 contributors after which github's server times out.

The only part remaining on this PR

@kilemensi
Copy link
Member

Using github graphql to fetch ...
The only part remaining on this PR

Thanks for double-checking this @koechkevin. Lets pick this up tomorrow when @kelvinkipruto is back to hear if he has a plan to deal with this issue or not.

@koechkevin
Copy link
Contributor

@kelvinkipruto check on this push to reduce API calls to github

@kelvinkipruto
Copy link
Contributor Author

@koechkevin Looks good. I have made a few updates to it.

@kilemensi
Copy link
Member

👍🏽

--

  1. Rename the icons to follow the current naming scheme. It allows us to see the size, color, etc., of the icon at a glance without opening it.
  2. I see some of the icons have different size/viewBox e.g. discord is 25/"0 0 25 25". If an icon size is not 24 (the default in MUI), you must set viewBox prop on the SVGIcon component so that MUI scales it accordingly.

Have these been addressed @kelvinkipruto ?

@kelvinkipruto
Copy link
Contributor Author

@kilemensi Updated.

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀


Lets deploy this to dev but still not happy with the code... something are supposed to be card but they're not, they way we're handling viewBox, etc.

@kelvinkipruto kelvinkipruto added this pull request to the merge queue Feb 29, 2024
Merged via the queue into main with commit 187754f Feb 29, 2024
4 checks passed
@kelvinkipruto kelvinkipruto deleted the ft/design-updates branch February 29, 2024 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants