Add new Banner prop to handle actions layout edge cases
#7045
Conversation
Co-authored-by: Jon Rohan <[email protected]>
🦋 Changeset detectedLatest commit: c4dbb5b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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
bot
added
the
integration-tests: recommended
|
👋 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
bot
requested a deployment
to
storybook-preview-7045
langermank
added
the
update snapshots
github-actions
bot
removed
the
update snapshots
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new actionsLayout prop to the Banner component to provide explicit control over action button layout, addressing edge cases where the default responsive behavior is undesirable (e.g., banners inside dialogs or select panels).
Key changes:
- Added
actionsLayoutprop with options:'default','inline', and'stacked' - Implemented CSS styling to support the new layout modes
- Removed the
BannerDescriptionclassName assignment (replaced with direct className prop pass-through)
Reviewed Changes
Copilot reviewed 7 out of 25 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Banner.tsx | Added actionsLayout prop definition, default value, and data attribute |
| Banner.test.tsx | Added unit tests verifying the data-actions-layout attribute for all three layout modes |
| Banner.module.css | Implemented CSS rules for inline/stacked layouts and updated selectors to respect actionsLayout |
| Banner.features.stories.tsx | Added story examples demonstrating the new prop in dialogs and at different viewport sizes |
| Banner.docs.json | Added documentation for the new actionsLayout prop |
| Banner.test.ts | Added VRT test coverage for the new layout modes |
| grumpy-walls-cry.md | Added changeset documenting this minor version change |
packages/react/src/Banner/Banner.tsx
Outdated
| export function BannerDescription({children, className, ...rest}: BannerDescriptionProps) { | ||
| return ( | ||
| <div {...rest} className={clsx('BannerDescription', className)}> | ||
| <div {...rest} className={className}> |
There was a problem hiding this comment.
The removal of the 'BannerDescription' class from this component appears unrelated to the stated PR purpose of adding actionsLayout functionality. This change could affect existing styles that depend on the BannerDescription class selector and should either be explained in the PR description or moved to a separate PR.
| <div {...rest} className={className}> | |
| <div {...rest} className={clsx(className, classes.BannerDescription)}> |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I removed it cause it doesn't have any corresponding CSS but I could make that change separately if it feels risky
github-actions
bot
requested a deployment
to
storybook-preview-7045
3 participants
Closes https://github.com/github/primer/issues/5897
This is another approach to this PR, which had some code smells checking for line length. I hammered on this issue for a few hours trying to find a CSS only approach, but it seems we have too many edge cases and complexity to how we want actions to wrap or swap ordering.
I paired with @jonrohan and we landed on adding a new prop to support this. The prop essentially lets consumers opt out of the current responsive behavior, setting the action buttons to either inline or stacked. This is useful for dialogs or select panel where we may render a small banner but still want the "desktop" experience.