-
Notifications
You must be signed in to change notification settings - Fork 645
Remove use of useResponsiveValue hook - SegmentedControl #7134
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?
Remove use of useResponsiveValue hook - SegmentedControl #7134
Conversation
🦋 Changeset detectedLatest commit: 993431c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
c36b54a to
7aa11ea
Compare
7aa11ea to
0f1a1ef
Compare
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 refactors the SegmentedControl component to use CSS-based responsive behavior via getResponsiveAttributes instead of the runtime useResponsiveValue hook. This change moves responsive logic from JavaScript to CSS, enabling more efficient rendering.
Key Changes:
- Replaced
useResponsiveValuehook withgetResponsiveAttributesutility forfullWidthandvariantprops - Removed JavaScript logic that transformed Buttons to IconButtons for hideLabels variant; now handled via CSS by hiding
.Textelements - Always renders both dropdown and segmented control variants when dropdown is used at any breakpoint, with CSS controlling visibility
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| SegmentedControl.tsx | Replaced useResponsiveValue with getResponsiveAttributes; removed hideLabels transformation logic; refactored rendering to always output both dropdown and segmented control when dropdown variant is present |
| SegmentedControl.module.css | Added comprehensive responsive styles for full-width and variant behaviors across all breakpoints; added CSS variable for icon width; added DropdownContainer styles |
| SegmentedControl.responsive.stories.tsx | Added new story file demonstrating responsive fullWidth, hideLabels, and dropdown variants across breakpoints |
| .changeset/segmentedcontrol-responsive.md | Added changeset documenting the change |
Co-authored-by: Copilot <[email protected]>
Closes https://github.com/github/primer/issues/6027
This implementation should fix all flickering caused by hydration mismatches.
Changelog
Unfortunately, there isn't a great way to reduce this repetition in standard CSS while maintaining the media query scoping. The pattern requires:
This structure can't easily be compressed because:
The current code is actually following the established pattern in the codebase (like in Hidden.module.css).
While repetitive, it's explicit and maintainable, and colocated.
New
Stories
segmentedcontrol-responsive-tests--complex-responsive
Screen.Recording.2025-11-07.at.15.34.56.mov
segmentedcontrol-responsive-tests--full-width-responsive
Screen.Recording.2025-11-07.at.15.35.24.mov
segmentedcontrol-responsive-tests--variant-dropdown-responsive
Screen.Recording.2025-11-07.at.15.35.46.mov
segmentedcontrol-responsive-tests--variant-hide-labels-responsive
Screen.Recording.2025-11-07.at.15.36.16.mov
Changed
SegmentedControl.module.cssto support responsivefullWidthandvariantprops using data attributes, enabling different layouts (e.g., full width, dropdown, hide labels) at narrow, regular, and wide breakpoints.SegmentedControl.tsxto conditionally render dropdown and segmented button variants based on responsive props, and removed the previous use ofuseResponsiveValue.Rollout strategy
Testing & Reviewing
Merge checklist