-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
feat: Prerender sponsors for READMEs #638
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for es-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for hi-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for zh-hans-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ja-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for pt-br-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for new-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for de-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for fr-eslint 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.
This is a good start. We also need to make sure that this script is run whenever the website is built.
I will make the requested changes. |
@amareshsm are you still working on this? |
Yes, I will push the request changes in some time. |
includes/generate-sponsors.js
Outdated
<p>${sponsors | ||
.map( | ||
sponsor => | ||
`<a href="${sponsor.url || "#"}"><img src="${ |
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.
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.
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.
The logo isn't missing on the website, and that's using the same data, so I'm not sure why this is creating a missing image. 🤔
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.
Perhaps you were using an old data file. Rebasing this branch and running the script again to regenerate sponsors.md might fix this.
includes/generate-sponsors.js
Outdated
const SPONSORS_URL = path.resolve(__dirname, "../src/_data/sponsors.json"); | ||
const TECH_SPONSORS_URL = path.resolve( |
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.
Can we rename these to SPONSORS_FILE_PATH
and TECH_SPONSORS_FILE_PATH
as these are not URLs but file paths? Also, can we rename fetch* functions to read* as they're not fetching data from URLs but reading files?
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.
sure
@nzakas in #638 (comment), did you mean that the generated file should be |
Co-authored-by: Milos Djermanovic <[email protected]>
Yes, it seems that the generated file should be placed inside the |
Co-authored-by: Nicholas C. Zakas <[email protected]>
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.
LGTM. Would like @mdjermanovic to review before merging.
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.
LGTM. Once please run the script before committing. will leave it open for Milos.
@@ -9687,42 +9726,6 @@ | |||
"url": "https://github.com/sponsors/ljharb" | |||
} | |||
}, | |||
"node_modules/got": { |
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.
Any idea why this was removed? The only change in package.json dependencies was adding common-tags
, which doesn't have any dependencies.
Prerequisites checklist
What is the purpose of this pull request?
Prerender sponsors for READMEs
What changes did you make? (Give an overview)
I added a script that retrieves sponsor data from JSON files and formats it into HTML sections for a markdown file. This script categorizes sponsors by tier and incorporates technology sponsors with their respective images. It then saves the formatted content to a new markdown file,
sponsors.md
, which we will utilize in our other repositories.Related Issues
#619
Is there anything you'd like reviewers to focus on?
Please let me know if this approach works and I'm open to any suggestions/changes.