Skip to content

chore: Refactor components to use BoxWithFallback instead of custom Box if statements and remove defaultSxProp references - #6278

Merged
jonrohan merged 5 commits into
mainfrom
refactor_use_boxwithfallback
Jul 7, 2025
Merged

chore: Refactor components to use BoxWithFallback instead of custom Box if statements and remove defaultSxProp references#6278
jonrohan merged 5 commits into
mainfrom
refactor_use_boxwithfallback

Conversation

@jonrohan

@jonrohan jonrohan commented Jul 3, 2025

Copy link
Copy Markdown
Member

I wanted to go through all the components using defaultSxProp and see if we can refactor them to use BoxWithFallback which is essentially doing the same check sxProp != defaultSxProp.

Benefits

  • Cleans up the codepaths where we might have 2 implementations of a component to render
  • Prepares the components more for when we're going to remove sxProp support completely
  • Less uses of Box component

AI summary of changes

  • Updated CircleBadge snapshots to reflect class name changes.
  • Refactored CounterLabel to use BoxWithFallback and removed defaultSxProp.
  • Modified DataTable's TableContainer to utilize BoxWithFallback.
  • Changed Details component to use BoxWithFallback instead of Box.
  • Updated FormControl to replace Box with BoxWithFallback.
  • Refactored Header, InlineMessage, Pagehead, Radio, SegmentedControl, and SubNav components to use BoxWithFallback.
  • Simplified Spinner component by using BoxWithFallback.
  • Updated Timeline components to utilize BoxWithFallback for consistency.
  • Removed defaultSxProp references across various components for cleaner code.

Changelog

New

Changed

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

…eferences

- Updated CircleBadge snapshots to reflect class name changes.
- Refactored CounterLabel to use BoxWithFallback and removed defaultSxProp.
- Modified DataTable's TableContainer to utilize BoxWithFallback.
- Changed Details component to use BoxWithFallback instead of Box.
- Updated FormControl to replace Box with BoxWithFallback.
- Refactored Header, InlineMessage, Pagehead, Radio, SegmentedControl, and SubNav components to use BoxWithFallback.
- Simplified Spinner component by using BoxWithFallback.
- Updated Timeline components to utilize BoxWithFallback for consistency.
- Removed defaultSxProp references across various components for cleaner code.
@changeset-bot

changeset-bot Bot commented Jul 3, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5d2d0db

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

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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 github-actions Bot added the staff Author is a staff member label Jul 3, 2025
@github-actions

github-actions Bot commented Jul 3, 2025

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-actions github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jul 3, 2025
@github-actions
github-actions Bot temporarily deployed to storybook-preview-6278 July 3, 2025 02:46 Inactive
@github-actions

github-actions Bot commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 92.02 KB (-0.38% 🔽)
packages/react/dist/browser.umd.js 92.23 KB (-0.28% 🔽)

@primer-integration

Copy link
Copy Markdown

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/389116

@github-actions
github-actions Bot requested a deployment to storybook-preview-6278 July 7, 2025 19:55 Abandoned
export const Description: React.FC<React.PropsWithChildren<ActionListDescriptionProps>> = ({
variant = 'inline',
sx = defaultSxProp,
sx,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I removed the default assignment here because the BoxWithFallback component also will set sx = defaultSxProp

as: Component = 'h3',
className,
children,
sx = defaultSxProp,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Anywhere where the props were spreading anyways like this {...rest} I removed the argument. I felt this was cleaner and will help us later when we drop the sx support (only needing to remove the SxProp type)

@primer-integration

Copy link
Copy Markdown

🟢 golden-jobs completed with status success.

@github-actions github-actions Bot added integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh and removed integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Jul 7, 2025
@jonrohan
jonrohan marked this pull request as ready for review July 7, 2025 20:16
@jonrohan
jonrohan requested a review from a team as a code owner July 7, 2025 20:16

@joshblack joshblack left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Phew, what a refactor 💯 Just left a couple style comments but honestly not too important. Thanks for doing this!

Comment thread packages/react/src/Details/Details.tsx Outdated
Comment thread packages/react/src/SubNav/SubNav.tsx Outdated
Comment thread packages/react/src/VisuallyHidden/VisuallyHidden.tsx Outdated
Co-authored-by: Josh Black <joshblack@github.com>
@github-actions github-actions Bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Jul 7, 2025
@github-actions

github-actions Bot commented Jul 7, 2025

Copy link
Copy Markdown
Contributor

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

@francinelucca francinelucca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Merged via the queue into main with commit 29facae Jul 7, 2025
@jonrohan
jonrohan deleted the refactor_use_boxwithfallback branch July 7, 2025 21:11
@primer primer Bot mentioned this pull request Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm staff Author is a staff member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants