Skip to content

Remove Box usage and sx prop from PageHeader - #6873

Closed
mperrotti wants to merge 7 commits into
mainfrom
mp/rm-box-and-sx-from-pageheader
Closed

Remove Box usage and sx prop from PageHeader#6873
mperrotti wants to merge 7 commits into
mainfrom
mp/rm-box-and-sx-from-pageheader

Conversation

@mperrotti

@mperrottimperrotti commented Sep 17, 2025

Copy link
Copy Markdown
Contributor

Relates to #6770 , https://github.com/github/primer/issues/5424 and https://github.com/github/primer/issues/5425

Changelog

New

Changed

sx prop was removed from PageHeader and its subcomponents

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

CopilotAI review requested due to automatic review settings September 17, 2025 22:40
@mperrotti
mperrotti requested a review from a team as a code ownerSeptember 17, 2025 22:40
@changeset-bot

changeset-botBot commented Sep 17, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: e0b38ac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
NameType
@primer/reactMajor
@primer/styled-reactMajor

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

@github-actions

Copy link
Copy Markdown
Contributor

👋 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!

@github-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Sep 17, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

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 PageHeader and its subcomponents while maintaining backward compatibility by creating a wrapped version in the styled-react package. The change supports the migration away from Box usage and the sx prop system as part of a design system modernization effort.

Key changes:

  • Removes sx prop usage from PageHeader components in the main react package
  • Creates backward-compatible wrapped components in styled-react package that restore sx support
  • Refactors PageHeader components to use modern polymorphic patterns

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
FileDescription
packages/react/src/PageHeader/PageHeader.tsxRemoves sx prop from Title component and refactors components to use modern polymorphic patterns
packages/styled-react/src/components/PageHeader.tsxCreates wrapped PageHeader components with sx prop support for backward compatibility
packages/styled-react/src/index.tsxUpdates exports to use the new wrapped PageHeader from styled-react instead of the main react package
packages/styled-react/src/sx.tsAdds utility function for handling sx prop styling
packages/styled-react/src/types/AriaRole.tsAdds AriaRole type definitions copied from the main react package

) as PolymorphicForwardRefComponent<'a', ParentLinkProps>
ParentLink.displayName = 'ParentLink'
function UnwrappedParentLink<TAs extends React.ElementType = 'a'>(
props: React.PropsWithChildren<ChildrenPropTypes & LinkProps & PolymorphicProps<TAs, 'a'>>,

CopilotAISep 17, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LinkProps type parameter is now generic but the function signature still uses the non-generic version. This should be LinkProps<TAs> to maintain type consistency with the generic parameter.

Suggested change
props: React.PropsWithChildren<ChildrenPropTypes&LinkProps&PolymorphicProps<TAs,'a'>>,
props: React.PropsWithChildren<ChildrenPropTypes&LinkProps<TAs>&PolymorphicProps<TAs,'a'>>,

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +23
// @ts-expect-error - PrimerPageHeader.Title is not recognized as a valid component type
return <Box as={PrimerPageHeader.Title} ref={ref} {...props} />

CopilotAISep 17, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using @ts-expect-error to suppress TypeScript errors indicates a type system issue that should be resolved rather than suppressed. Consider properly typing the component or using a more specific type assertion.

Suggested change
// @ts-expect-error - PrimerPageHeader.Title is not recognized as a valid component type
return<Boxas={PrimerPageHeader.Title}ref={ref}{...props}/>
return<Boxas={PrimerPageHeader.TitleasReact.ElementType}ref={ref}{...props}/>

Copilot uses AI. Check for mistakes.
})

// weird typecast to get around mysterious TS error
const PageHeader = Object.assign(PrimerPageHeader as unknown as React.FC<PageHeaderProps>, {

CopilotAISep 17, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type assertion as unknown as React.FC<PageHeaderProps> is overly broad and potentially unsafe. Consider using a more specific type or properly typing the PageHeader component to avoid this casting.

Copilot uses AI. Check for mistakes.
@github-actions
github-actionsBot requested a deployment to storybook-preview-6873 September 17, 2025 22:47 Abandoned

@liuliu-devliuliu-dev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey 👋 It looks like PageHeader is already covered in this PR (primer/react#6871). Feel free to close this one if that’s the case!

@mperrotti

Copy link
Copy Markdown
ContributorAuthor

Closing because PageHeader was handled by #6871

@primer
primerBot deleted the mp/rm-box-and-sx-from-pageheader branch June 30, 2026 18:09
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommendedThis change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@mperrotti@liuliu-dev