-
Notifications
You must be signed in to change notification settings - Fork 646
Banner: Add flush prop for use inside dialogs, sidebars and other confined spaces
#7250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 0514121 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 |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
Add a `flush` prop to the Banner component for better usage in confined spaces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a flush prop to the Banner component for use within confined spaces like dialogs, sidebars, tables, cards, or boxes. The flush variant removes the left and right borders and border-radius to create a full-width appearance that integrates better with container boundaries.
Key changes:
- Adds boolean
flushprop with default value offalseto Banner component - Applies CSS styling to remove left/right borders and border-radius when flush is true
- Includes a story demonstrating the flush banner inside a Dialog component
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packages/react/src/Banner/Banner.tsx |
Adds flush prop definition, default value, and data attribute implementation |
packages/react/src/Banner/Banner.module.css |
Adds CSS selector for data-flush='true' to remove side borders and border-radius |
packages/react/src/Banner/Banner.features.stories.tsx |
Updates InsideDialog story to FlushInsideDialog demonstrating the flush prop usage with custom Dialog renderBody |
packages/react/src/Banner/Banner.docs.json |
Adds documentation for the flush prop with type, default value, and description |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
francinelucca
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! non-blocking suggestions
| "name": "flush", | ||
| "type": "boolean", | ||
| "defaultValue": "false", | ||
| "description": "Full width banner specifically for use within confined spaces, such as dialogs, tables, cards, or boxes where available space is limited." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(you might be onto this already) Should we also plan to add some guidance around when flush is acceptable to our docs? 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep! Lukas has a PR open
Co-authored-by: Marie Lucca <40550942+francinelucca@users.noreply.github.com>
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
lukasoppermann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. 👍
|
👋 Hi, there are new commits since the last successful integration test. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
Rollout strategy
Testing & Reviewing
Merge checklist