[ButtonGroup] Determine first, last and middle buttons to support different elements with correct styling

[ZeeshanTamboli]

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

Based on point 2 of #29514 (comment).

Fixes #31210
Fixes #29514

Decided to tackle this after reviewing PR #38403.

Before: https://codesandbox.io/s/lively-http-3w24nw?file=/src/App.tsx
After: https://codesandbox.io/s/quizzical-glitter-kgrmyn

[mui-bot]

Netlify deploy preview

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

@material-ui/core: parsed: +0.11% , gzip: +0.12%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against c5f8b76

[siriwatknp]

@ZeeshanTamboli Instead of exposing the className, how about using just data-* attribute like:

• data-first-child
• data-last-child

No need for middle because we can use CSS :not to target middle children like this:
.${buttonGroupClasses.root}:not([data-first-child], [data-last-child])

[ZeeshanTamboli]

@siriwatknp I had thought about that and also referred Joy UI's ButtonGroup component code. The reason I added middle class is because then the :not selector will select the wrapping elements instead of button. As an example, in the case of the Tooltip shown in the CodeSandbox provided in the description it will select the Tooltip's span elements for:

<ButtonGroup>
  <Tooltip title="tooltip">
    <Button>Enabled</Button>
  </Tooltip>
  <Tooltip title="tooltip">
    <span>
      <Button disabled>Disabled</Button>
    </span>
  </Tooltip>
  <Tooltip title="tooltip">
    <span>
      <Button disabled>Disabled</Button>
    </span>
  </Tooltip>
</ButtonGroup>

And span is also needed to show the tooltip on disabled buttons - https://mui.com/material-ui/react-tooltip/#disabled-elements. Related issue #29514. The :not selector, however, won't target nested elements, such as a button inside a tooltip.

Regarding exposing classes, offering customization options to users might be a good idea. However, if desired, I could also use data-first-child, data-middle-child, and data-last-child attributes. Let me know your preference.

[siriwatknp]

Alright, let's try data-middle-child instead of classes so that we don't expose too much API at this point.

[ZeeshanTamboli]

@siriwatknp Done.

[michaldudak]

@siriwatknp, why not classes? We tend to use classes everywhere in Material UI for styling.

[ZeeshanTamboli]

I rolled back to use classes now.

[oliviertassinari]

@ZeeshanTamboli Please don't ping the core group for PR reviews that are on sub-product scopes. This creates too much notification noise.

Instead, this is a PR for the ButtonGroup, you can ask for a review from one of the maintainers https://www.notion.so/mui-org/Components-Inputs-ecd48c9afe354bda8ecda3e96abc8c3c.

Thanks

[DiegoAndai]

Hey @ZeeshanTamboli! sorry for the late review, I left some questions

[ZeeshanTamboli]

Can this PR be reviewed further?

[DiegoAndai]

@ZeeshanTamboli

Can this PR be reviewed further?

The logic looks good to me, but I agree with @michaldudak points:

• Using classes seems to be the way to implement something like this in Material UI. Data attributes would be unexpected for users.
• With this in mind, we should add tests to ensure the classes are applied correctly. The regression test won't cover all cases. For example, imagine we implement this using certain CSS classes and later refactor to rename them. If the refactoring is done well, the regression test will continue to work, but the classes might have changed, which might break the apps of users relying on them.

[ZeeshanTamboli]

@DiegoAndai I made the changes, switching from data attributes to class names. I've also relocated the styles from the MuiButtonGroup-grouped className. This change may appear substantial in the code diff. We avoided targeting the first, middle, and last class names within the grouped classname to prevent increasing specificity, making it easier for users to override styles using the theme's styleOverrides or the sx prop. Previously, we applied styles using :first-of-type and :last-of-type selectors only to buttons with the MuiButtonGroup-grouped classname so it had to be inside the grouped class. This is no longer the case.

Additionally, I've included unit tests. So, if I understand correctly, these tests are added to ensure that we don't unintentionally introduce breaking changes when modifying the classes, is that right?

[DiegoAndai]

@ZeeshanTamboli thanks for applying the changes 😊

I agree with the specificity change 👍🏼

These tests are added to ensure that we don't unintentionally introduce breaking changes when modifying the classes, is that right?

Exactly.

I've added below another idea that came to mind.

[DiegoAndai]

LGTM 🚀

This should go on the highlights of next release, nice improvement!