-
Notifications
You must be signed in to change notification settings - Fork 631
Remove Box usage and sx
prop from UnderlineNav
#6869
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
🦋 Changeset detectedLatest commit: d41cab6 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 |
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 removes the sx
prop from UnderlineNav
and its subcomponents as part of the migration away from styled-components. The changes introduce a CSS Modules-based styling approach while maintaining the component's functionality.
Key Changes:
- Removes
sx
prop support fromUnderlineNav
andUnderlineNavItem
components - Introduces CSS Modules for styling with
UnderlineNav.module.css
- Adds a modern polymorphic utility for better type inference with the
as
prop - Creates styled-react compatibility layer to maintain backward compatibility
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/react/src/UnderlineNav/UnderlineNav.tsx |
Removes Box usage and sx prop, replaces with CSS Modules classes |
packages/react/src/UnderlineNav/UnderlineNavItem.tsx |
Migrates from Box/sx to modern polymorphic approach with CSS classes |
packages/react/src/UnderlineNav/UnderlineNav.module.css |
New CSS Modules file defining component styles |
packages/react/src/utils/modern-polymorphic.ts |
New utility for improved polymorphic component types |
packages/styled-react/src/components/UnderlineNav.tsx |
Compatibility wrapper to maintain sx prop support in legacy package |
packages/react/src/UnderlineNav/UnderlineNav.test.tsx |
Updates tests to use new function names and constants |
packages/react/src/UnderlineNav/UnderlineNav.docs.json |
Removes sx prop from documentation |
// typecasting to get around a confusing mismatch between `as` props | ||
// between UnderlineNavItem and UnderlineItem | ||
as={as as React.ElementType} |
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 type casting as={as as React.ElementType}
suggests a type system mismatch. Consider aligning the type definitions between UnderlineNavItem and UnderlineItem to eliminate the need for this workaround.
Copilot uses AI. Check for mistakes.
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.
Tried it, couldn't figure it out
box-shadow: | ||
/* TODO: replace custom shadow with Primer Primitives variable */ | ||
/* stylelint-disable-next-line primer/box-shadow */ | ||
rgba(0, 0, 0, 0.12) 0 1px 3px, | ||
rgba(0, 0, 0, 0.24) 0 1px 2px; |
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 TODO comment indicates this custom shadow should be replaced with a Primer Primitives variable. Consider creating a follow-up issue to address this technical debt.
box-shadow: | |
/* TODO: replace custom shadow with Primer Primitives variable */ | |
/* stylelint-disable-next-line primer/box-shadow */ | |
rgba(0, 0, 0, 0.12) 0 1px 3px, | |
rgba(0, 0, 0, 0.24) 0 1px 2px; | |
box-shadow: var(--shadow-overlay); |
Copilot uses AI. Check for mistakes.
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.
No, this would be the wrong style
👋 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! |
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.
packages/react/src/utils/modern-polymorphic.ts had me second guessing it, as we're adding yet another abstraction (so more complexity in a way) but it's simple enough that it seems useful.
Nice!
Relates to https://github.com/github/primer/issues/5424 and https://github.com/github/primer/issues/5425
Changelog
New
Changed
sx
prop was removed fromUnderlineNav
and its subcomponentsRemoved
Rollout strategy
Testing & Reviewing
Merge checklist