-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
feat: introduce StatelessSelect #7976
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 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
Introduce a noscript-compatible fallback for the existing Select component so that users without JavaScript can still choose navigation or version options via native HTML.
- Added a new
StatelessSelect
component wrapping<details>
/<summary>
for non-JS environments - Updated the main
Select
to renderStatelessSelect
under<noscript>
and generated a dynamic class hook - Extended Sidebar and CSS modules to support the fallback markup and styling
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
packages/ui-components/src/Containers/Sidebar/index.tsx | Forwarded the new as prop into the mobile Select invocation |
packages/ui-components/src/Common/Select/index.tsx | Exported helpers, added an as prop, generated a unique class, and integrated StatelessSelect |
packages/ui-components/src/Common/Select/index.module.css | Added .noscript styles and nested rules for the fallback UI |
packages/ui-components/src/Common/Select/StatelessSelect/index.tsx | New component implementing a native <details> /<summary> fallback |
Comments suppressed due to low confidence (3)
packages/ui-components/src/Common/Select/StatelessSelect/index.tsx:1
- This new
StatelessSelect
component does not appear to have any unit tests. Consider adding tests to cover its rendering and behavior (e.g., mapping values, default selection, and correct markup in a<noscript>
environment).
import { ChevronDownIcon } from '@heroicons/react/24/solid';
packages/ui-components/src/Containers/Sidebar/index.tsx:52
- The
as
prop is being passed to the Select component but isn’t being destructured from props in this component. You should includeas
in the function’s parameters (e.g.,({ as, ... })
) so it’s defined.
as={as}
packages/ui-components/src/Common/Select/StatelessSelect/index.tsx:107
- This always passes an
href
attribute toComponent
, even ifas
is'div'
. Rendering ahref
on a<div>
is invalid HTML and can cause React warnings. Consider conditionally addinghref
only whenComponent
supports it or restrictas
to anchor-like elements.
<Component
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #7976 +/- ##
==========================================
- Coverage 73.07% 73.01% -0.07%
==========================================
Files 95 96 +1
Lines 8358 8359 +1
Branches 218 218
==========================================
- Hits 6108 6103 -5
- Misses 2249 2255 +6
Partials 1 1 ☔ View full report in Codecov by Sentry. |
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 it possible to have a story for statelessSelect
?
packages/ui-components/src/Common/Select/StatelessSelect/index.tsx
Outdated
Show resolved
Hide resolved
packages/ui-components/src/Common/Select/StatelessSelect/index.tsx
Outdated
Show resolved
Hide resolved
packages/ui-components/src/Common/Select/StatelessSelect/index.tsx
Outdated
Show resolved
Hide resolved
No, or at least not on its current code, that's why IMO the |
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, under the conditions that a story is added.
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 ! that's clean
Description
For users visiting the site with JavaScript disabled, navigating through pages and downloading the desired version should remain one of the website’s core capabilities.
To support this, I’ve implemented a fallback version of our current select component using native HTML
<details>
and<summary>
elements. While this version is less accessible than the JavaScript-enhanced version, it remains functional without relying on JavaScriptValidation
In the preview, with JavaScript disabled on mobile viewports:
The navigation select on the Learn pages should correctly redirect to the appropriate pages.
Similarly, the version select on the Download page should be properly rendered, but should not perform any redirection.
Related Issues
Unblock #7794
Check List
pnpm format
to ensure the code follows the style guide.pnpm test
to check if all tests are passing.pnpm build
to check if the website builds without errors.