[material-ui][ButtonGroup] Renders incorrectly when rendering conditionally from custom component.

[AlexShukel]

Duplicates

• [X] I have searched the existing issues

Latest version

• [X] I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://codesandbox.io/s/hopeful-rubin-yskwss?file=/src/App.js

Current behavior 😯

Single button rendered without right border.

Expected behavior 🤔

Button should have all borders as it is a single visible children in ButtonGroup.

Context 🔦

This issue is related to https://github.com/mui/material-ui/issues/38978 This behavior has been broken from version 5.14.9, see this comment for more info.

Your environment 🌎

System: OS: Windows 10 10.0.22621 Binaries: Node: 18.12.1 - C:\Program Files\nodejs\node.EXE Yarn: 1.22.19 - C:\Program Files\nodejs\node_modules\corepack\shims\yarn.CMD npm: 8.19.2 - C:\Program Files\nodejs\npm.CMD Browsers: Chrome: Not Found Edge: Chromium (118.0.2088.46)

[DiegoAndai]

Hey @AlexShukel, thanks for the report!

What would you think about a workaround like this one: https://codesandbox.io/s/39488-possible-workaround-k86xxf

This seems like an advanced use case, so the idea would be to modify the button group button context in userland based on the visible condition for the second button.

We could add an example for this in the docs.

cc: @ZeeshanTamboli

[AlexShukel]

@DiegoAndai this is not a workaround because first button must know whether the second button is visible or not. We need a single MyButton component (abstraction on top of a Button), what implies that it cannot know whether it's siblings visible or not.

[DiegoAndai]

implies that it cannot know whether it's siblings visible or not

Why is this a limitation? Could you share more details about your use case in which it's not possible to create another abstraction for the first button?

This is a difficult issue to solve on Material UI's side as the edge cases are very specific to the use case. The ButtonGroup should handle the primary case (buttons are direct children) and be extendable for other cases on the developer's side. This is what I'm proposing. The specific extension I proposed may not be enough, so having more info about your use case might help develop a better one.

[AlexShukel]

@DiegoAndai In my use case there is an abstraction ButtonWithPrivilege, which takes the information about it's visibility from the context, which is provided with some values from the server. The application code looks like this: https://codesandbox.io/s/eager-waterfall-wm2w5q?file=/src/App.js

[ZeeshanTamboli]

I believe this is a clear regression because it was functional before version 5.14.9 (prior to the release of #38520), as I mentioned in https://github.com/mui/material-ui/issues/38978#issuecomment-1766469170. We weren't aware of this specific use case.

As far as I know, the only solution is to use CSS selectors for styling based on the rendered DOM button elements in v6 (you can find more details in https://github.com/mui/material-ui/issues/38978#issuecomment-1766469170). Currently, we are relying on checking valid JSX elements within the button group. This had to be done to solve other issues. If somebody has a better solution with Emotion JS i.e in v5 without any workarounds it would be great.

[DiegoAndai]

Ok, I'll add the regression and ready-to-take label. For anyone taking this:

• Have in mind #38978 (comment). It might be the case that we need to wait for Emotion-less v6. If that's the case, let us know so we can mark it as planned for v6.
• If we need to wait until v6, we should look into how to support an acceptable workaround for this use case.

[Na-hyunwoo]

@DiegoAndai Hello, I would like to solve this problem, can I give it a try?

[DiegoAndai]

@Na-hyunwoo hi! Sure, give it a go.