-
Notifications
You must be signed in to change notification settings - Fork 10
feat(buttons): introduce button group with sub variants #588
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
|
Hi @chintankavathia, whenever you have some time, would you be so kind as to help me generate the VRTs for this pull request as well? Thanks a lot in advance. |
|
Documentation. Coverage Reports: |
seems like some ally issue |
4ac44d3 to
343bdbf
Compare
343bdbf to
f54d81b
Compare
chintankavathia
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.
@dauriamarco focus ring doesn't seem to be placed correctly when compared to figma specs.

figma:
also the dropdown items focus ring is clipped from both sides
f54d81b to
6986ae5
Compare
|
@chintankavathia I might be wrong, but to me it looks like the Figma example is slightly off. Otherwise, it would mean that the focus state of any other button is currently incorrect in the code. What do you say @hbxes? |
6986ae5 to
64d5795
Compare
spike-rabbit
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.
I think we actually need to wait here, until we have the new button radios, as this will affect the code here
ce55efa to
267d98a
Compare
|
@dauriamarco the figma file in terms of the focus is fixed |
...snapshots/static.spec.ts-snapshots/buttons--buttons-element-examples-chromium-dark-linux.png
Show resolved
Hide resolved
Thanks @kfenner 🙌 I’ll try adding the VRTs again, so far I’ve had issues running them locally and had to ask others to generate them for me, but I’ll give it another shot later on my machine to see if things are working now. On the naming: I wasn’t fully decided between the two. I went with selection-buttons because it felt a bit more generic and aligned with the functionality of the button group itself, but I haven’t had a definitive discussion with anyone yet. Totally fine to revisit this and align with iX if needed. |
a993129 to
d03b1f7
Compare
...ots/static.spec.ts-snapshots/buttons--button-groups-element-examples-chromium-dark-linux.png
Show resolved
Hide resolved
d03b1f7 to
c1c94df
Compare
382d6df to
903e82b
Compare
903e82b to
b3add63
Compare
b136667 to
3204be0
Compare
ae6d1b2 to
0a7ee5a
Compare
0a7ee5a to
ee92126
Compare
kfenner
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.
LGMT 👍
addressed
|
🎉 This PR is included in version 48.3.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |


Adds the button group with sub-variants: button-groups, split-button, toolbar, and also renames the segmented-button.
Here's some screenshots for a quick overview:
Related to #250