-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Deprecate CrowdinContributors, GitHubContributors #12969
Conversation
deprecate redundant CrowdinContributors and GitHubContributors components; consolidate logic to gather file contributors list into `utils/contributors.ts` and use to fetch/organize all data in getStaticProps before passing to layout/components.
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @wackerow! a very useful refactor.
Left a few minor comments related to the getFileContributorInfo
function but nothing blocking.
locale: string, | ||
fileLang: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a scenario where these two are different?
mdDir: string, | ||
mdPath: string, | ||
slug: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I see too many args for this function. For example, mdDir
and mdPath
are derived from slug
. Perhaps we could abstract that logic (how to calc mdDir and mdPath) somewhere in the utils and reduce the noise form this function.
IMO, ideally, this function should only require 2 args: slug and locale (the cache is arguable but ok).
slug: string, | ||
locale: string, | ||
fileLang: string, | ||
layout: Layout, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we use this arg to determine whether or not we want to fetch the crowdin contributors. Couldn't we deduce this from the slug? slug.includes("/developers/")
Sub-branch PR:
get-static-contributors <- get-static-contributors-consolidation
Description
contributors
that will be passed to the layout and components from[...slug].tsx
.contributors.ts
util file