-
Notifications
You must be signed in to change notification settings - Fork 638
Remove Box usage and sx
prop from Link and misc other areas (stories, etc)
#6825
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
🦋 Changeset detectedLatest commit: bb77600 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! |
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 Box
component usage and sx
prop from multiple components as part of a major migration. The changes eliminate the legacy styled-components dependency and styled system support, transitioning components to use CSS modules and modern polymorphic patterns instead.
Key Changes:
- Removes
sx
prop support from components like Announce, AriaStatus, Dialog, Heading, Link, NavList, PageLayout, and UnderlineNav - Replaces
Box
component wrapping with direct HTML element rendering or CSS modules - Implements modern polymorphic component patterns using
fixedForwardRef
andPolymorphicProps
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
packages/styled-react/src/types/AriaRole.ts | Adds AriaRole type definitions for backwards compatibility |
packages/styled-react/src/sx.ts | Creates sx utility function for styled-components compatibility layer |
packages/styled-react/src/index.tsx | Updates exports and creates wrapped components with sx support |
packages/styled-react/src/components/ | New component wrappers that restore sx prop functionality |
packages/react/src/utils/modern-polymorphic.ts | New utility for modern polymorphic component patterns |
packages/react/src/*/index.ts | Updates component exports and type definitions |
packages/react/src//.tsx | Component implementations migrated from Box+sx to direct elements+CSS modules |
packages/react/src//.module.css | New CSS module styles replacing sx-based styling |
packages/react/src//.docs.json | Documentation updates removing deprecated sx prop references |
packages/react/src//.test.tsx | Test updates removing sx-related test cases |
// @ts-expect-error - PrimerPageLayout is not recognized as a valid component type | ||
return <Wrapper as={PrimerPageLayout} ref={ref} {...props} /> |
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 @ts-expect-error comment indicates a type system issue that should be properly resolved rather than suppressed. Consider creating proper type definitions for PrimerPageLayout or using a more specific typing approach to avoid the need for error suppression.
Copilot uses AI. Check for mistakes.
export type UnderlineItemProps<As extends React.ElementType = 'a'> = { | ||
as?: As | ||
className?: string | ||
iconsVisible?: boolean | ||
loadingCounters?: boolean | ||
counter?: number | string | ||
icon?: FC<IconProps> | React.ReactElement | ||
id?: string | ||
ref?: React.Ref<unknown> | ||
} & SxProp | ||
children?: React.ReactNode | ||
} & SxProp & | ||
PolymorphicProps<As, 'a'> |
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 definition includes both SxProp
and modern polymorphic props. Since this PR is removing sx
support, the SxProp
should be removed from this type definition to maintain consistency with the migration goals.
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.
Stop judging me, Copilot. That's a separate issue. We'll get to it...
size-limit report 📦
|
…react into mp/rm-box-and-sx-from-components
This PR will pass integration tests with the following changes to |
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
sx
prop from Heading, Link, and misc areassx
prop from Link and misc other areas (stories, etc)
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
packages/styled-react/src/__tests__/primer-react.browser.test.tsx
Outdated
Show resolved
Hide resolved
👋 Hi, there are new commits since the last successful integration test. We recommend running the integration workflow once more, unless you are sure the new changes do not affect github/github. Thanks! |
packages/styled-react/src/__tests__/primer-react.browser.test.tsx
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,35 @@ | |||
// Mostly taken from https://github.com/total-typescript/react-typescript-tutorial/blob/main/src/08-advanced-patterns/72-as-prop-with-forward-ref.solution.tsx |
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.
can you elaborate on the need for this over the regular polymorphic we've been using?
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.
Our current approach causes the following problems:
- I don't think there's a good way to use
ForwardRefComponent
without casting (as ForwardRefComponent
) - We can't access the prop types to automatically generate component API docs (project @adierkens is working on) when we cast with
as ForwardRefComponent
- The
ForwardRefComponent
type isn't portable, so I get an error when I try to import a component that usesForwardRefComponent
intostyled-react
- The
DistributiveOmit
(usually) handles type collisions better (for example: we specify anonClick
, but the native HTML element type has a conflictingonClick
) - I think the
ForwardRefComponent
approach is less type-safe, but I don't remember the specifics
@adierkens or somebody who is actually good at TypeScript can keep me correct on 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.
All that said, I'm fine with going back to ForwardRefComponent
for now 🙂
This just felt like a good opportunity to level-up our component types.
Relates to https://github.com/github/primer/issues/5424 and https://github.com/github/primer/issues/5425
Closes https://github.com/github/primer/issues/5714
Changelog
New
Changed
Removed
sx
support was removed from LinkRollout strategy
Testing & Reviewing
Merge checklist