-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
feat: add toggle to show/hide LTS releases #8263
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 adds a toggle switch to filter non-LTS releases from the EOL (End of Life) table. Users can now hide non-LTS versions to focus on LTS releases only.
Key Changes:
- Introduced a new Switch UI component with Radix UI integration
- Refactored EOL table to support client-side filtering with state management
- Fixed the
isLtsdetermination logic to useltsStartinstead of status string matching
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-components/src/Common/Switch/index.tsx | New Switch component implementation using Radix UI primitives |
| packages/ui-components/src/Common/Switch/index.stories.tsx | Storybook stories for the Switch component |
| packages/ui-components/src/Common/Switch/index.module.css | Styling for the Switch component with light/dark mode support |
| packages/ui-components/package.json | Added @radix-ui/react-switch dependency |
| packages/i18n/src/locales/en.json | Added translation key for "Hide non-LTS versions" label |
| apps/site/next-data/generators/releaseData.mjs | Fixed isLts logic to check for ltsStart existence |
| apps/site/components/EOL/EOLReleaseTable/index.tsx | Refactored to use new client component |
| apps/site/components/EOL/EOLReleaseTable/TableClient.tsx | New client component with filtering state and Switch integration |
| apps/site/components/EOL/EOLReleaseTable/TableBody.tsx | Removed file - functionality merged into TableClient |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
avivkeller
left a comment
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.
Wow! Thanks for this!
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
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Thanks for the quick review! |
flakey5
left a comment
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 for web infra bits, thank you!
AugustinMauroy
left a comment
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.
LGMT !
bmuenzenmeyer
left a comment
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8263 +/- ##
==========================================
- Coverage 76.72% 76.26% -0.46%
==========================================
Files 118 118
Lines 9805 9903 +98
Branches 335 336 +1
==========================================
+ Hits 7523 7553 +30
- Misses 2280 2348 +68
Partials 2 2 ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
bjohansebas
left a comment
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, it looks cool to be able to do that
|
Lighthouse Results
|
|
@ovflowd can you re review |
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 very unusual component name. Why TableClient? Because it is a client Component? Unfortunately that's an awful name 😅
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.
Yep, that was my reasoning. Seeing that I got multiple comments on it, it was a bad call 😅 I will rename it after we agree on the other changes about the table
This is now reverted
| checked={hideNonLts} | ||
| onCheckedChange={setHideNonLts} | ||
| /> | ||
| <table id="tbVulnerabilities"> |
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.
Why did you move this here and rename the file? Could you please revert this change? :)
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.
Reverted
|
|
||
| return ( | ||
| <> | ||
| <Switch |
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.
My gut feeling tells me you can do a pure HTML switch with CSS and have the table being hidden purely with CSS.
Can you please do that? On this website we want to avoid feature-gating behind JavaScript.
| versionWithPrefix: `v${latestVersion.semver.raw}`, | ||
| codename: major.support.codename || '', | ||
| isLts: status.endsWith('LTS'), | ||
| isLts: support.ltsStart !== undefined, |
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 has unwanted side-effects. This flag isn't used to say if a version is a LTS version-line or not, but if it is currently LTS. Please revert this change and use the codename as a field to verify a release line is a LTS release line.
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.
Whoops, reverted this back + using codename for checking for LTS releases now
| @@ -0,0 +1,49 @@ | |||
| 'use client'; | |||
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.
As much as I love Radix! I feel we should make the switch purely with CSS and HTML and have the table entries be affected based on that.
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.
Fair, I changed the implementation to use html/css only. However, the only way I could get it to work was through using has, which is newly available (https://caniuse.com/css-has). Does this work?
ovflowd
left a comment
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.
Appreciate your contribution! Thanks for working on this :)
I've left a few comments.
8fe558b to
070fe96
Compare
8c1eb71 to
4cd0fa0
Compare
|
Thanks for the review @ovflowd, I've changed the implementation to use html/css only. The changes to the data sources is also reverted (as they were wrong in the first place). Could you unblock your review @bmuenzenmeyer ? |
| {eolReleases.map(release => ( | ||
| <Fragment key={release.major}> | ||
| <tr> | ||
| <tr data-lts={!!release.codename}> |
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.
smart!
|
I cannot get prettier to overwrite the current contents, which makes me think the failure is a false positive or caching problem |
Howdy! I'm on work travel, I'll be reviewing this later today or tomorrow! |

Description
Adds a toggle to the EOL table that filters non LTS releases
Validation
Content being filtered:

Other:



Related Issues
Closes #8017
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.