Skip to content
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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

lee-chase
Copy link
Member

@lee-chase lee-chase commented Feb 28, 2024

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 the ButtonSet component.

With the fluid property set to true the button set exhibits the following behavior

  • Controls the inline-size of the buttons
  • Auto stacks the buttons based on $min-inline-button-size: convert.to-rem(176px)
  • Re-orders buttons based on guidance
  • Adds withDisplayBox to allow control of container width in storybook
  • Adds SetOfButonsFluid story

Further work

  • Setting a default button set in the story is not functional. NOTE: A default not functional elsewhere e.g. MenuButton Playground MenuAlignment.
  • Notify users (via event handler) of stacking change.
  • Allow for gaps between buttons
  • Control auto stack with a property.

Changelog

New

  • packages/react/.storybook/templates/WithDisplayBox/WithDisplayBox.scss
  • packages/react/.storybook/templates/WithDisplayBox/index.js
  • packages/react/src/components/Button/story/fluid-button-set-args.js

Changed

  • packages/react/src/components/Button/Button.stories.js
  • packages/react/src/components/ButtonSet/ButtonSet.tsx
  • packages/styles/scss/components/button/_button.scss

Testing / Reviewing

Checked functional in storybook

@lee-chase lee-chase requested a review from a team as a code owner February 28, 2024 17:23
Copy link

netlify bot commented Feb 28, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ba82d66
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/65df6bfcd464160008281fad
😎 Deploy Preview https://deploy-preview-15843--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Feb 28, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit bffc866
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6644e88992cd26000859673a
😎 Deploy Preview https://deploy-preview-15843--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@lee-chase lee-chase requested a review from a team as a code owner February 28, 2024 18:16
@alisonjoseph alisonjoseph requested review from a team and thyhmdo and removed request for a team February 28, 2024 20:37
@tw15egan
Copy link
Member

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 320px? It seems to grow until 520px and then becomes a non-fluid variant, but I'm just trying to determine the correct spec. Similarly, what should they grow to in a 2, 3, or 4-button group? The 4 button group has them all stack at a 700px width

@lee-chase
Copy link
Member Author

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 320px? It seems to grow until 520px and then becomes a non-fluid variant, but I'm just trying to determine the correct spec. Similarly, what should they grow to in a 2, 3, or 4-button group? The 4 button group has them all stack at a 700px width

Not sure I have an answer to that. The sizes and behaviour chosen were very much a reflection of the ActionSet used in @carbon/ibm-products SidePanel. The values chosen in the case of SidePanel were judged to be aesthetically pleasing for the SidePanel rather than a fluid button set by itself.

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.

@thyhmdo
Copy link
Member

thyhmdo commented May 1, 2024

Hi, sorry for the late review. This looks great. I have a few questions and concerns:

  • This experience is different from the Set of Buttons. I was confused for 5 sec not knowing what to do. It could populate an option upfront.
  • From interacting with the "Stacked" prop, I'm not sure I see differences, unless I'm missing something.
  • @lee-chase Can the visual change here if I provide a spec? I was a little bit confused about how this worked since we don't have this experience anywhere in the storybook.

@lee-chase
Copy link
Member Author

@thyhmdo apologies for the lack of clarity.

  • I did try top populate this up front, but had some difficulty with Storybook, which seems to behave differently to IBM Products. Perhaps one of the devs could advise, happy to update. The simplest thing to do would be to move the various fluid button options into a folder.
  • Stacked - if I'm honest I do not recall why I left this in this story. I have removed it and can see no issues in doing so.
  • Not sure what you are asking with your final query.

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.
image

@thyhmdo
Copy link
Member

thyhmdo commented May 2, 2024

@thyhmdo apologies for the lack of clarity.

  • I did try top populate this up front, but had some difficulty with Storybook, which seems to behave differently to IBM Products. Perhaps one of the devs could advise, happy to update. The simplest thing to do would be to move the various fluid button options into a folder.
  • Stacked - if I'm honest I do not recall why I left this in this story. I have removed it and can see no issues in doing so.
  • Not sure what you are asking with your final query.

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. image

  1. @tw15egan @guidari Do you have suggestions for this?
  2. Nice! I checked and it seems like the problem is solved.
  3. Similar to components with layers, Anna provided some specs to have a particular look for it. In this visual, the label is quite long, the width extension could be replaced with something else that is close to our design spec (see image-red line)
  • The label could be simplified, maybe something like: "Max width (component container)
  • The other label could be "Width varies based on the max width adjustment"
image

@guidari
Copy link
Contributor

guidari commented May 2, 2024

Hey @lee-chase
I did a commit on your PR to populate an option up front like @thyhmdo mentioned.

https://deploy-preview-15843--v11-carbon-react.netlify.app/?path=/story/components-button--set-of-buttons-fluid

@thyhmdo
Copy link
Member

thyhmdo commented May 7, 2024

Hey @lee-chase I did a commit on your PR to populate an option up front like @thyhmdo mentioned.

https://deploy-preview-15843--v11-carbon-react.netlify.app/?path=/story/components-button--set-of-buttons-fluid

that looks good! thanks @guidari

@lee-chase lee-chase requested a review from a team as a code owner May 15, 2024 16:53
Copy link
Contributor

@guidari guidari left a 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. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants