-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat: add fluid button set #15843
base: main
Are you sure you want to change the base?
feat: add fluid button set #15843
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
This is great, @lee-chase. Thanks for taking the time to add this in. I had a question: Should the single button max out at |
Not sure I have an answer to that. The sizes and behaviour chosen were very much a reflection of the ActionSet used in Given that custom CSS properties cannot be used in media queries (container or otherwise) perhaps these value could be defined as SASS variables or part of a mixin call to allow a consumer could override should they need to. |
Hi, sorry for the late review. This looks great. I have a few questions and concerns:
|
@thyhmdo apologies for the lack of clarity.
The wrapper around the button story is something done in IBM Products and could probably do with a UX tidy up. The intent is to show the component but provide a simple mechanism to allow the user to see how the component behaves with different container sizes. |
|
Hey @lee-chase |
that looks good! thanks @guidari |
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.
Tested and that looks good to me. 🚀
@andreancardona I do not believe this is waiting for response from me. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey, thanks again for your work here! Had a brief discussion with the team about any blockers we see preventing this from merging. One thing that came up was the container slider. We've had users struggle to understand and use controls inside stories like this. We'd like to move away from them where possible. One idea is to instead have a new custom toolbar item in the storybook toolbar. It would contain a list of px values stepping in multiples of 160px. On the code level I worry about determining if the buttons are currently stacked by imperatively evaluating the styles applied via carbon/config/jest-config-carbon/setup/setup.js Lines 33 to 35 in 184d1a9
Sorting the buttons feels like something we would typically leave up to the consumer. I see the intended benefit, but using Children methods immediately limits the structure of what consumers can provide there. I'm not sure the benefits outweigh that cost? |
@tay1orjones & team Switching from the container slider to a toolbar item is a possibility, but would need visual input as to how that is represented in the story. The viewport control has a similar behaviour and clearly marks out the usable area. Whether or not the buttons are stacked is currently used to reposition box shadows and sorting. Even if sorting is deferred to the user they would need to monitor when stacking had occurred. In an ideal world we would provide If there is no possibility of a tooltip (currently icon button only) then |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15843 +/- ##
==========================================
- Coverage 82.87% 82.73% -0.14%
==========================================
Files 404 405 +1
Lines 14338 14391 +53
Branches 4591 4624 +33
==========================================
+ Hits 11882 11906 +24
- Misses 2296 2323 +27
- Partials 160 162 +2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
The documentation on buttons https://carbondesignsystem.com/components/button/usage/
mentions fluid buttons and how they're handled. However, this does not appear to be implemented.
This PR adds the
fluid
property to theButtonSet
component.With the
fluid
property set to true the button set exhibits the following behavior$min-inline-button-size: convert.to-rem(176px)
withDisplayBox
to allow control of container width in storybookSetOfButonsFluid
storyFurther work
Changelog
New
Changed
Testing / Reviewing
Checked functional in storybook