Skip to content

bugfix(Spinner): Spinner component size get's large when passing sx and size props - #5236

Merged
jonrohan merged 5 commits into
mainfrom
spinner_box_fix
Nov 6, 2024
Merged

bugfix(Spinner): Spinner component size get's large when passing sx and size props#5236
jonrohan merged 5 commits into
mainfrom
spinner_box_fix

Conversation

@jonrohan

@jonrohanjonrohan commented Nov 5, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Closeshttps://github.com/github/primer/issues/4391

This fixes an issue with the Spinner and CSS modules we discovered this morning. The problem is when sx= and size= is passed to the Spinner component, we render <Box as={Spinner} {...props} /> but Box accepts a size= prop as well as Spinner so Box ends up creating some styles thinking that it's meant to render at different breakpoints.

To fix I figured we need to not pass size to Box. So to fix it, I ended up removing Box and using a styled.div component with nothing but ${sx} inside to render any sx props passed along.

This fixed the issue. We created a dev story to recreate the issue.

Changelog

New

Changed

Spinner component size get's large when passing sx and size props

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

@changeset-bot

changeset-botBot commented Nov 5, 2024•
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 293ca50

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

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

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-actionsgithub-actionsBot added the staff Author is a staff member label Nov 5, 2024
@github-actions

github-actionsBot commented Nov 5, 2024

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 Nov 5, 2024
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5236 November 5, 2024 20:17 Inactive
@github-actions

github-actionsBot commented Nov 5, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

size-limit report 📦

PathSize
packages/react/dist/browser.esm.js98.5 KB (-0.01% 🔽)
packages/react/dist/browser.umd.js98.88 KB (-0.03% 🔽)

@github-actions
github-actionsBottemporarily deployed to storybook-preview-5236 November 5, 2024 21:19 Inactive
@jonrohanjonrohan changed the title Bug fix styled spinnerbugfix(Spinner): Spinner component size get's large when passing sx and size propsNov 5, 2024
@jonrohan
jonrohan marked this pull request as ready for review November 5, 2024 21:24
@jonrohan
jonrohan requested a review from a team as a code ownerNovember 5, 2024 21:24
@github-actions
github-actionsBottemporarily deployed to storybook-preview-5236 November 5, 2024 21:26 Inactive
@primer-integration

primer-integrationBot commented Nov 5, 2024

Copy link
Copy Markdown

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

joshblack
joshblack requested changes Nov 5, 2024
View reviewed changes
Comment thread.changeset/forty-rats-jog.md Outdated
Comment threadpackages/react/src/Spinner/Spinner.dev.stories.tsx Outdated
jonrohanand others added 2 commits November 5, 2024 13:30
@jonrohan@joshblack
Update .changeset/forty-rats-jog.md
27631c9
Co-authored-by: Josh Black <joshblack@github.com>
@jonrohan@joshblack
Update packages/react/src/Spinner/Spinner.dev.stories.tsx
293ca50
Co-authored-by: Josh Black <joshblack@github.com>
jonrohan
jonrohan commented Nov 5, 2024
View reviewed changes
Comment threadpackages/react/src/Spinner/Spinner.tsx
}

return <Spinner className={classes.SpinnerAnimation} {...props} />
return <Spinner className={clsx(className, classes.SpinnerAnimation)} {...props} />

@jonrohanjonrohanNov 5, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Not related to the bug, but was pointed out that className was overwriting the CSS modules classname when being passed in, so I added this

@github-actions
github-actionsBottemporarily deployed to storybook-preview-5236 November 5, 2024 21:33 Inactive
@primer-integration

primer-integrationBot commented Nov 5, 2024

Copy link
Copy Markdown

🟢 golden-jobs completed with status success.

@jonrohan
jonrohan added this pull request to the merge queueNov 6, 2024
Merged via the queue into main with commit 2e5c601Nov 6, 2024
@jonrohan
jonrohan deleted the spinner_box_fix branch November 6, 2024 19:47
@primerprimerBot mentioned this pull request Nov 6, 2024
Merged
@siddharthkpsiddharthkp mentioned this pull request Nov 8, 2024
Closed
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hussam-i-amhussam-i-amhussam-i-am approved these changes
@joshblackjoshblackjoshblack approved these changes
@randall-krauskopfrandall-krauskopfrandall-krauskopf approved these changes
@keithamuskeithamusAwaiting requested review from keithamuskeithamus is a code owner automatically assigned from primer/engineer-reviewers

Assignees

No one assigned

Labels

integration-tests: recommendedThis change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpmstaffAuthor is a staff member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jonrohan@hussam-i-am@joshblack@randall-krauskopf