fix(Button): Apply inline-flex display to loading wrapper for link variant#7690
fix(Button): Apply inline-flex display to loading wrapper for link variant#7690liuliu-dev merged 5 commits intomainfrom
Conversation
…riant When a Button with variant="link" uses the loading prop, the data-loading-wrapper div now gets display: inline-flex to preserve inline behavior. Co-authored-by: HiroAgustin <1458873+HiroAgustin@users.noreply.github.com> Agent-Logs-Url: https://github.com/primer/react/sessions/54244975-bac3-490e-8119-69589a0d92dc
|
github-actions
bot
added
the
integration-tests: recommended
hectahertz
left a comment
hectahertz
left a comment
There was a problem hiding this comment.
Looks good! We may want to check integration tests just in case as it changes the layout
There was a problem hiding this comment.
Pull request overview
Fixes inline layout regressions for Button when variant="link" is used together with the loading prop by ensuring the loading wrapper can be inline-level.
Changes:
- Add a new CSS Module class to make the loading wrapper
display: inline-flexfor link-variant buttons. - Conditionally apply the new wrapper class in
ButtonBasewhenvariant === 'link'. - Add a unit test and a patch changeset entry for the behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/Button/ButtonBase.tsx | Applies conditional wrapper class for link variant when loading wrapper is present. |
| packages/react/src/Button/ButtonBase.module.css | Adds .ConditionalWrapperLink { display: inline-flex; } for the wrapper. |
| packages/react/src/Button/tests/Button.test.tsx | Verifies the loading wrapper receives the link wrapper class. |
| .changeset/fix-link-loading-wrapper.md | Adds patch changeset documenting the fix. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 7b2063e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
🤖 Lint and formatting issues have been automatically fixed and committed to this PR. |
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: HiroAgustin <1458873+HiroAgustin@users.noreply.github.com>
|
Deployed storybook looks good: 
I opened the playground, checked loading and selected variant link and added some inline text next to the button. |
|
Uh oh! @HiroAgustin, at least one image you shared is missing helpful alt text. Check #7690 (comment) to fix the following violations:
Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/16663 |
TylerJDev
added
the
integration-tests: skipped manually
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Buttonwithvariant="link"setsdisplay: inline-flexon the button itself, but whenloadingis used, the button is wrapped in a<div data-loading-wrapper>that defaults to block display, breaking inline layout.This adds a
ConditionalWrapperLinkCSS class (display: inline-flex) applied to the wrapper whenvariant === 'link'.Changelog
New
Changed
ButtonBase.module.css: Added.ConditionalWrapperLink { display: inline-flex }classButtonBase.tsx: Conditionally apply wrapper class viaclsx(block && classes.ConditionalWrapper, variant === 'link' && classes.ConditionalWrapperLink)Removed
Rollout strategy
Testing & Reviewing
Unit test added verifying the loading wrapper receives the
ConditionalWrapperLinkclass whenvariant="link".Merge checklist