-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
fix(site): close release modal on eol link click #7903
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Pull Request Overview
This PR fixes the bug where the release modal remains open when the user clicks the "Understand EOL support" link.
- Added an onClick handler (closeModal) to the Link component to close the modal on click.
- Updated the href to point to "/about/previous-releases#release-schedule" for proper navigation.
Comments suppressed due to low confidence (1)
apps/site/components/Downloads/ReleaseModal/index.tsx:45
- [nitpick] Consider evaluating if the onClick handler should prevent default navigation (e.g., via event.preventDefault) to ensure the modal is fully closed before the browser navigates to the new anchor. This can help prevent any potential race conditions between modal teardown and navigation.
onClick={closeModal}
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #7903 +/- ##
==========================================
- Coverage 75.47% 75.40% -0.08%
==========================================
Files 101 96 -5
Lines 8311 8350 +39
Branches 218 219 +1
==========================================
+ Hits 6273 6296 +23
- Misses 2036 2052 +16
Partials 2 2 ☔ View full report in Codecov by Sentry. |
|
Nit, @araujogui could you:
|
In https://github.com/nodejs/node/blob/main/CHANGELOG.md, they are just "Node.js {ver}" |
Yes, but in other places of the website we use v{x}, just want to keep it standard. (And that image is not within our domain/realm) |
|
To be clear, I'm fine to be either Node v{x}; Node.js v{x}; or Node.js {x}; or Node {x}-- I just want consistency across the board :) |
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.
TYSM <3
|
Lighthouse Results
|
|
I'm fast-tracking this as a bug-fix. FYI @nodejs/nodejs-website |


Description
Fixes a bug where the release modal didn't close when the user clicked the 'Understand EOL support' link.
Validation
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.