-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Collapse borders between buttons in outlined ButtonGroup #7005
base: develop
Are you sure you want to change the base?
Conversation
Generate changelog in
|
Apply border collapsing behavior to buttons in outlined ButtonGroupBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Add split button demo to ButtonGroup popover exampleBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
&:not(.#{$ns}-vertical) { | ||
> .#{$ns}-popover-target:not(:last-child) > .#{$ns}-button, | ||
> .#{$ns}-button:not(:last-child) { | ||
border-right: none; | ||
} |
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.
Changes to this file include a subset of the same changes originally applied in #6966, but scoped only to the outlined
ButtonGroup.
|
||
@reactExample ButtonGroupExample | ||
|
||
@## Usage | ||
|
||
Most of __ButtonGroup__'s props are also supported by __Button__ directly; setting these props on __ButtonGroup__ will | ||
Most of **ButtonGroup**'s props are also supported by **Button** directly; setting these props on **ButtonGroup** will |
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.
Formatting changes were added here by prettier on editor save.
7758916
to
c18e83c
Compare
Add split button demo to ButtonGroup popover exampleBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
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 - thanks for the docs updates to go along with this. Will approve when you're ready with the docs changes just had one small comment to consider.
__Button__ elements inside a __ButtonGroup__ can trivially be wrapped with a [__Popover__](#core/components/popover) to | ||
create complex toolbars. | ||
**Button** elements inside a **ButtonGroup** can be wrapped with a [**Popover**](#core/components/popover) to create | ||
complex toolbars or to provide split button functionality, allowing the action of a **Button** to be changed. |
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.
I don't think "split button" is the unique thing to call out here in the use with popovers section, maybe "menu button"?
Also not sure about allowing the action of a **Button** to be changed
- pretty much anything could be done inside the popover so it feels weird to call out a specific case here. I think it would be weird if the popover just contained options that would change the wrapped button's behavior (implying it needs to be clicked again?).
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.
Yeah, fair! I originally modeled this after GitHub's own merge options button, which I've seen as a pretty common dropdown use pattern in other places. Totally fine with omitting this and leaving the docs as is if the example isn't adding value.
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.
Reverted 7776357
This reverts commit c18e83c.
Revert "Add split button demo to ButtonGroup popover example"Build artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fixes #0000
Checklist
Changes proposed in this pull request:
This change applies a subset of functionality from #6966 (reverted in #7002) in order to collapse borders between Buttons in a ButtonGroup iff the
outlined
prop is applied on ButtonGroup. Conversely, the collapsed border behavior is not present ifoutlined
is applied to individual buttons within the ButtonGroup. This allows consumers to either opt-in or opt-out of the collapsed border behavior. It also prevents a regression in styles to the default ButtonGroup where consumers are already using mixed variant/intent buttons within a ButtonGroup.Example of where the collapsing border behavior does apply:
Examples of where the collapsing border behavior does not apply:
ButtonGroups with mixed use buttons (e.g.
minimal
andoutlined
) can also still co-exist if the variants are applied to individual Buttons:Split Buttons
Additionally, this allows consumers of Blueprint to specify a split button without the double border between buttons:
A "split button" example has been added to the "Usage with popovers" section to demonstrate this:
Reviewers should focus on:
All possible combinations of
<ButtonGroup>
variants and<Button>
children variants to avoid regressions in styles.