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

[ButtonGroup][joy] Fix style for single Button #37692

Merged

Conversation

MaybePixem
Copy link
Contributor

@MaybePixem MaybePixem commented Jun 24, 2023

[ButtonGroup][joy-ui] fix style for single child

Fixes #37687

Single buttons are now properly styled
grafik
grafik

Before: https://codesandbox.io/s/exciting-black-y2glhj
After: https://codesandbox.io/s/amazing-sid-pth57n?file=/demo.tsx

[ButtonGroup][joy-ui] fix style for single child
@mui-bot
Copy link

mui-bot commented Jun 24, 2023

Netlify deploy preview

https://deploy-preview-37692--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 8859155

@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 24, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [ButtonGroup][joy-ui] fix style for single child [ButtonGroup][joy] fix style for single child Jun 25, 2023
@ZeeshanTamboli ZeeshanTamboli changed the title [ButtonGroup][joy] fix style for single child [ButtonGroup][joy] Fix style for single Button Jun 25, 2023
@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work component: ButtonGroup The React component. and removed component: data grid This is the name of the generic UI component, not the React module! labels Jun 25, 2023
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request!

Unfortunately, it doesn't work for the second demo. You can find it at https://codesandbox.io/s/new-hooks-dmz3t2?file=/demo.tsx. The issue might be that the React.Children.count(children) count for the wrapped Button component could be two.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

I think you can use CSS selector :only-child instead of a new data-single-child. Here is the change I propose:

diff --git a/packages/mui-joy/src/ButtonGroup/ButtonGroup.tsx b/packages/mui-joy/src/ButtonGroup/ButtonGroup.tsx
index 3c9109e89f..8e07fcfd71 100644
--- a/packages/mui-joy/src/ButtonGroup/ButtonGroup.tsx
+++ b/packages/mui-joy/src/ButtonGroup/ButtonGroup.tsx
@@ -84,7 +84,7 @@ const ButtonGroupRoot = styled('div', {
       borderRadius: 'var(--ButtonGroup-radius)',
       flexDirection: ownerState.orientation === 'vertical' ? 'column' : 'row',
       // first Button or IconButton
-      [`& > [data-first-child]`]: {
+      [`& > [data-first-child]:not([data-last-child])`]: {
         '--Button-radius': firstChildRadius,
         '--IconButton-radius': firstChildRadius,
         ...(ownerState.orientation === 'horizontal' && {
@@ -109,7 +109,7 @@ const ButtonGroupRoot = styled('div', {
         }),
       },
       // last Button or IconButton
-      [`& > [data-last-child]`]: {
+      [`& > [data-last-child]:not([data-first-child]`]: {
         '--Button-radius': lastChildRadius,
         '--IconButton-radius': lastChildRadius,
         ...(ownerState.orientation === 'horizontal' && {
@@ -119,7 +119,7 @@ const ButtonGroupRoot = styled('div', {
           borderTop: 'var(--ButtonGroup-separatorSize) solid var(--ButtonGroup-separatorColor)',
         }),
       },
-      [`& > :not([data-first-child])`]: {
+      [`& > :not([data-first-child]):not(:only-child)`]: {
         '--Button-margin': margin,
         '--IconButton-margin': margin,
       },

@siriwatknp
Copy link
Member

Thank you for the pull request!

Unfortunately, it doesn't work for the second demo. You can find it at https://codesandbox.io/s/new-hooks-dmz3t2?file=/demo.tsx. The issue might be that the React.Children.count(children) count for the wrapped Button component could be two.

For the second demo, I'd say we should add a doc for Custom button and explain that we use React.cloneElement() so developers should spread the props to the button.

@MaybePixem
Copy link
Contributor Author

Good idea with the :only-child. Will add the changes

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

I am not very familiar with Joy UI code but your logic also seems to make sense compared to #37692 (review) and even custom button works without the need of spreading props because the data-attributes are not added for a single button.

packages/mui-joy/src/ButtonGroup/ButtonGroup.test.tsx Outdated Show resolved Hide resolved
packages/mui-joy/src/ButtonGroup/ButtonGroup.test.tsx Outdated Show resolved Hide resolved
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Awesome!

@siriwatknp siriwatknp merged commit a629ea1 into mui:master Jul 6, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: ButtonGroup The React component. package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Joy][ButtonGroup] Single button in a button group doesn't properly round corners
5 participants