fix(Banner): stack inline actions vertically on narrow viewports#7625
fix(Banner): stack inline actions vertically on narrow viewports#7625liuliu-dev merged 7 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: c614cbb 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 |
|
Uh oh! @liuliu-dev, at least one image you shared is missing helpful alt text. Check your pull request body 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.
|
github-actions
bot
added
the
integration-tests: recommended
|
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
There was a problem hiding this comment.
Pull request overview
Adjusts @primer/react’s Banner styling to prevent inline action buttons from overlapping at narrow viewport widths, addressing an accessibility-audits issue.
Changes:
- Add a narrow-viewport media rule for inline (
actionsLayout="inline") trailing actions to stack vertically. - Add a patch changeset entry for the Banner fix.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react/src/Banner/Banner.module.css | Changes inline trailing actions layout at narrow viewports to avoid button overlap. |
| .changeset/gold-rooms-bake.md | Declares a patch release for the Banner layout adjustment. |
| display: flex; | ||
|
|
||
| @media screen and (--viewportRange-narrow) { | ||
| flex-direction: column-reverse; |
There was a problem hiding this comment.
Using flex-direction: column-reverse will make the visual order of the actions differ from DOM/focus order (Banner renders secondaryAction before primaryAction in the trailing container). On narrow viewports this can cause keyboard focus to move bottom→top, which is an accessibility issue. Prefer flex-direction: column (to keep visual order aligned with focus order), or otherwise change the rendered DOM order for this breakpoint so visual and focus order match.
| flex-direction: column-reverse; | |
| flex-direction: column; |
There was a problem hiding this comment.
tested and this seems to be correct 👀 let's address it
There was a problem hiding this comment.
good catch on keyboard focus @copilot!
I initially used column-reverse to put the primary button on top, this is how it looks after switched to column to keep visual order aligned with focus order:
Curious what you think about this @lukasoppermann 👀
There was a problem hiding this comment.
The idea was actually that on small view ports the buttons are below:
Wonder why this is not working. Would it be possible to fix it like that? The text is very narrow if it is in two columns.
There was a problem hiding this comment.
the issue is specifically from the actionsLayout="inline" story, which overrides the default responsive behavior. From the docs:
When actionsLayout is inline, the container has nowrap which prevents wrapping at all viewport sizes. Should we allow inline to fall back to stacked at narrow viewports? or it should always stay inline?
There was a problem hiding this comment.
Confirmed with @lukasoppermann that we will stack on narrow view port:
inline is meant for the non-mobile view, where adding the x normally moves the buttons below, which can be bad if all you have is a very short title.
So for mobile, lets fall back to stacked.
| display: flex; | ||
|
|
||
| @media screen and (--viewportRange-narrow) { | ||
| flex-direction: column-reverse; |
There was a problem hiding this comment.
When switching this container to a column layout, the existing spacing defined on .BannerActionsContainer via column-gap won’t add vertical spacing between stacked buttons. Consider adding a row-gap/gap override for the narrow breakpoint (or switching the base container to gap) so the vertically stacked actions don’t touch.
| flex-direction: column-reverse; | |
| flex-direction: column-reverse; | |
| row-gap: var(--base-size-2); |
| @media screen and (--viewportRange-narrow) { | ||
| flex-direction: column-reverse; | ||
| } |
There was a problem hiding this comment.
This change fixes a narrow-viewport layout issue, but the existing Banner stories don’t currently cover actionsLayout="inline" at ~320px with both primary and secondary actions (the mobile example only shows a primary action). Adding/updating a Storybook case (and corresponding VRT/AVT coverage if applicable) for inline+two-actions at narrow widths would help prevent regressions.
francinelucca
left a comment
francinelucca
left a comment
There was a problem hiding this comment.
Lets address https://github.com/primer/react/pull/7625/changes#r2891377700
Would also be nice to add VRT on narrow for this if possible 👀
Aaaand can we get design to sign off on the new narrow visuals? 🙏🏽
liuliu-dev
added
the
update snapshots
github-actions
bot
removed
the
update snapshots
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/15651 |
liuliu-dev
added
integration-tests: passing
|
skipping integration tests as |
liuliu-dev
deleted the
liuliu/fix-banner-stack-actions-on-narrow-viewport
branch
Addresses issue #14971
Changelog
Fixes an overlap issue in
BannerwhenactionsLayout="inline"is used with both a primary and secondary action at narrow viewport (320×256px).Solution: At narrow viewports (
--viewportRange-narrow), the trailing actions container switches toflex-direction: column-reverse, stacking the primary and secondary action buttons vertically.Before:
After:
Rollout strategy
Testing & Reviewing
Merge checklist