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

[ToggleButton][Joy] Add ToggleButton component #37716

Merged
merged 25 commits into from
Jul 18, 2023

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 25, 2023

closes #36617

New

  • Joy UI does not provide a ToggleButton component (It's simple enough to do it on the developer side).
  • Joy UI provides a ToggleButtonGroup (using the same styles as the ButtonGroup) that can wrap Button and IconButton.
  • The pressed state uses the active variant styles.

@siriwatknp siriwatknp added component: toggle button This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy design: joy This is about Joy Design, please involve a visual or UX designer in the process labels Jun 25, 2023
@mui-bot
Copy link

mui-bot commented Jun 25, 2023

Netlify deploy preview

@mui/joy: parsed: +0.76% , gzip: +0.75%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against cc4b8b4

@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 25, 2023

Preview: https://deploy-preview-37716--material-ui.netlify.app/experiments/joy/toggle-button/

With box-shadow:

Screen.Recording.2566-06-25.at.12.06.14.mov

Without box-shadow:

Screen.Recording.2566-06-25.at.12.06.44.mov

@zanivan Which version do you prefer 👆?

@zanivan
Copy link
Contributor

zanivan commented Jun 26, 2023

@siriwatknp I'm just adding it here to keep it documented:

  • Go with the version without box-shadow
  • Use Active color on pressed state instead of hover

@siriwatknp
Copy link
Member Author

@zanivan it looks better with active background!

Screen.Recording.2566-06-27.at.09.33.54.mov

@siriwatknp
Copy link
Member Author

@zanivan Ready for the first review! https://deploy-preview-37716--material-ui.netlify.app/joy-ui/react-toggle-button/

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 6, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 12, 2023
@siriwatknp siriwatknp marked this pull request as ready for review July 12, 2023 09:36
@siriwatknp siriwatknp requested a review from zanivan July 12, 2023 09:36
@zanivan zanivan requested a review from samuelsycamore July 12, 2023 13:39
@zanivan
Copy link
Contributor

zanivan commented Jul 12, 2023

@siriwatknp It looks good! I've just added some tweaks to the texts. While I was reviewing, two things came to my mind:

  1. I wonder if on the first demo, we shouldn't use the same styling as the active? Though the idea of showing some customization is good, I believe that it'd be better to show the users how to use aria-pressed on buttons and keep the cohesive and consistent design across the lib.
  2. As we actually don't have a ToggleButton, and only a ToggleButtonGroup, wouldn't it be better to rename the page? When I read it for the first time, it was a bit awkward to start the Toggle Button page with Joy UI does not provide a ToggleButton component 😅

@siriwatknp
Copy link
Member Author

@siriwatknp It looks good! I've just added some tweaks to the texts. While I was reviewing, two things came to my mind:

  1. I wonder if on the first demo, we shouldn't use the same styling as the active? Though the idea of showing some customization is good, I believe that it'd be better to show the users how to use aria-pressed on buttons and keep the cohesive and consistent design across the lib.
  2. As we actually don't have a ToggleButton, and only a ToggleButtonGroup, wouldn't it be better to rename the page? When I read it for the first time, it was a bit awkward to start the Toggle Button page with Joy UI does not provide a ToggleButton component 😅

That makes a lot of sense. Let me update the page.

@siriwatknp
Copy link
Member Author

@zanivan I think it is ready!

Copy link
Contributor

@zanivan zanivan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe wait for a review from @danilo-leal or @samuelsycamore to polish out the copy a bit if needed, but it looks good to me! ✨

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! Gave it a shot at the docs formatting/editing but @samuelsycamore should definitely take a look everything's sounding good!

@siriwatknp siriwatknp merged commit 5131b59 into mui:master Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: toggle button This is the name of the generic UI component, not the React module! design: joy This is about Joy Design, please involve a visual or UX designer in the process package: joy-ui Specific to @mui/joy
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

[Joy] Add the ToggleButton component
4 participants