[material-next][ButtonGroup] Apply MD3 style to ButtonGroup

[lhilgert9]

• I have followed (at least) the PR section of the contributing guide.

ButtonGroup issue: #39686
Material You umbrella issue: #29345
Preview: https://deploy-preview-40124--material-ui.netlify.app/material-ui/react-button-group/#material-3-version

Changes

• Change color props
• Change variant props
• Apply MD3 style
• Add playground to docs
• Remove combined classes and styleOverrides-keys

[mui-bot]

Netlify deploy preview

• docs/data/material/components/button-group/button-group.md

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 743c851

[DiegoAndai]

Thanks for working on this @lhilgert9, and sorry for the late review.

I left a couple of questions.

Besides that, I will assign @zanivan for a design review. Victor, could you help us here with filling the Material 3 specs gaps? The specs only specify styles for the outlined variant:

But we need to adapt it to other variants. I think what Lucas did with the other variants is looking good:

But I think we could use some borders in between. I'm curious to see if you come up with anything else we might want to add to these. You can test out the different variants in the playground.

[DiegoAndai]

We shouldn't remove the inherit and other color options

[lhilgert9]

@DiegoAndai I had removed these because the other colors were also removed in the button. https://github.com/mui/material-ui/blob/master/packages/mui-material-next/src/Button/Button.types.ts#L40

[DiegoAndai]

True. May I ask you to bring those back? We can do it in this PR

[lhilgert9]

@DiegoAndai 'error' | 'info' | 'success' | 'warning' are no problem to implement, but 'inherit' is very expensive to implement, because at every point where the property color is called, a check must first be made to ensure that it is not inherit and then the special forms for inherit must be implemented. So the question is whether you really want to keep inherit and whether it is worth the effort?

[DiegoAndai]

That's a good point; it does seem like a specific use case that bloats the styles—tagging @mui/core to get more opinions.

[DiegoAndai]

I discussed with some Core members and realized that the inherit color is pretty useful when dealing with composed components like navigation bars, modals, and alerts.

Is there a way we can improve the implementation so it's not so "expensive"?

[lhilgert9]

Okay, that sounds logical.
However, due to the complexity, I would suggest implementing the whole thing in a new PR, in which the missing colors are then returned for both the button and the button group. Here you could possibly also solve the problems with the color change of the button, which occurs when you use filledTonal or elevated as a variant and is only used primary as a color.

[zanivan]

Hey there! Joining late to the party, but I'll leave my review from the design aspect of it:

This is how I believe the variants should look like—I tried to use the button and the divider as a base for it:

moreover, I believe the states (hovered, disabled, etc) should follow the same as buttons!

Thanks for tackling this, @lhilgert9! 🚀

[lhilgert9]

@zanivan Everything should be implemented. Please have a look and give me some feedback. Regarding the hovered state, I'm not sure whether we should implement this in the same way as the button, as this looks strange in my eyes. However, you don't see a big change in the state as I have currently implemented it. Any ideas on this?

Original Button implementation:

• filled

• filledTonal

My Button implementation:

• filled (I think this is okay)

• filledTonal (this is not okay, because you see no change)

[zanivan]

It looks good!

Please, correct me if I'm wrong, but I suspect that the current implementation of the hover is not how it is supposed to be (judging by the design resources). If I understood i it correctly, currently we're only using the opacity layer, so that's why it seems unchanged on the demo.

Since we're using the opacity (0.8, I believe) the background will influence directly on the contrast. See when we change it to black, for instance:

White background	Black background

---

The right approach, as I see, would be to implement it as it's on the design guidelines—at least on Figma kit—where we have a layer using a solid background color, and while :hover we add an opacity layer over. For example:

@DiegoAndai This seems to be happening on the current Material You button too. Maybe we should solve there first?

[DiegoAndai]

I suspect that the current implementation of the hover is not how it is supposed to be (judging by the design resources).

You're correct. We noticed it during the Chip implementation. On that component, it's implemented properly using color-mix: https://github.com/mui/material-ui/blob/master/packages/mui-material-next/src/Chip/Chip.tsx#L253

We need to port this to other components that are already implemented, I created an issue for it (but it is on hold for now): #40517

I would merge this in its current state and resume it when we start working on v7. How does that sound, @lhilgert9 @zanivan?

[lhilgert9]

@DiegoAndai That's okay with me, since it's only an experimental package, so bugs can still occur and it doesn't look bad per se.

[zanivan]

I would merge this in its current state and resume it when we start working on v7

Sure, I think we can wait since more components are affected the same way.