mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.91k stars 32.27k forks source link

[joy-ui][ButtonGroup] Broken styles with custom button elements #39372

Open kamavingah opened 1 year ago

kamavingah commented 1 year ago

Duplicates

Latest version

Steps to reproduce 🕹

https://codesandbox.io/s/affectionate-wiles-xzrrzp?file=/Demo.tsx

  1. Create custom Button:
const A = () => {
  return <Button>hi</Button>;
};
  1. Use Button in ButtonGroup
<ButtonGroup>
  <A />
  <A />
</ButtonGroup>

Current behavior 😯

The borders of the ButtonGroup are not rounded.

Expected behavior 🤔

The borders of the ButtonGroup should be rounded.

Context 🔦

No response

Your environment 🌎

@mui/joy 5.0.0-beta.9

vineetjk commented 1 year ago

@PhillipWinder you are correct, And one more thing I observed -> if <ButtonGroup> has only one child then the style is correct.

vineetjk commented 1 year ago

if this issue is ready to take, can I work on this?

danilo-leal commented 1 year ago

@vineetjk go for it! 🙌

vineetjk commented 1 year ago

sure @danilo-leal, I will work on this

vineetjk commented 1 year ago

Hi @brijeshb42 , I am able to run the docs locally in my machine. But I am facing a problem while reproducing the bug in playground. PFA -> image image

Kindly guide me if I am in wrong direction.

Thanks

vineetjk commented 1 year ago

Hi @brijeshb42 , I am able to run the docs locally in my machine. But I am facing a problem while reproducing the bug in playground. PFA -> image image

Kindly guide me if I am in wrong direction.

Thanks

This problem solved by wrapping playground page with CssVarsProvider

import { CssVarsProvider } from '@mui/joy/styles';

 <CssVarsProvider>
   …
 </CssVarsProvider>
vineetjk commented 1 year ago

I have found the cause for this bug.. 'data-first-child' and 'data-last-child' props are not applied to the buttons which are returned by function or wrapped in <> ...</>

ZeeshanTamboli commented 1 year ago

Indeed, it should function with custom buttons. You can make it work by spreading the props to the Button element using this code:

const A = (props) => {
  return <Button {...props}>hi</Button>;
};

You can see an example here: CodeSandbox Link.

I believe the solution is to utilize the React Context API instead of cloneElements for ButtonGroup. This way, the Joy UI Button component can apply the data-* attributes to the DOM even when wrapped with a custom element.

vineetjk commented 1 year ago

Hi @ZeeshanTamboli, You are correct! it works if we spread props to the Button element. But the same issue is not observed in material library example : https://codesandbox.io/s/stupefied-montalcini-h8t9tc

ZeeshanTamboli commented 1 year ago

But the same issue is not observed in material library example : https://codesandbox.io/s/stupefied-montalcini-h8t9tc

Indeed, that's why I suggested using the Context API; it's what Material UI relies on internally.

vineetjk commented 1 year ago

Yeah I got it, sure I will do the changes.. thanks :)

aacevski commented 8 months ago

I wrote on the newer issue but just to make sure but I'll write here just in case, I have opened a PR proposal, I managed to fix it by just using CSS.

https://github.com/mui/material-ui/pull/41456