material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

IconButton has unnecessary padding when ripple is disabled

Open TiagoPortfolio opened this issue 3 years ago • 3 comments

Duplicates

  • [X] I have searched the existing issues

Latest version

  • [X] I have tested the latest version

Current behavior 😯

When we use the IconButton component, it has the ripple effect by default and padding to display that effect. If we disable the ripple effect, the IconButton will still have that padding and cause alignment issues.

<IconButton
  size="small"
  disableRipple
>

image As you can see in the image above, the button is not aligned on the right with the text field because of that padding.

Expected behavior 🤔

After disabling the ripple effect for the IconButton, I was expecting the padding to be removed to be easier to do alignment: image

Steps to reproduce 🕹

In this example you can see that the icon buttons are not aligned with the text field at the top: https://codesandbox.io/s/holy-rain-hwthkz?file=/demo.tsx

Context 🔦

The IconButton component has the ripple effect by default and padding to display that effect. That padding is not helpful to align with other content and after disabling the ripple effect for the IconButton, I was expecting the padding to be removed to be easier to do alignment but it wasn't.

Should we add that logic into the component to remove the padding if the ripple effect is disabled or should we handle that with the sx prop? I don't mind working on a PR for this change if you think it makes sense :)

Your environment 🌎

npx @mui/envinfo
  Used Chrome Browser

   System:
    OS: macOS 12.5
  Binaries:
    Node: 16.13.1 - /usr/local/bin/node
    Yarn: 1.22.17 - /usr/local/bin/yarn
    npm: 8.5.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 104.0.5112.79
    Edge: Not Found
    Firefox: Not Found
    Safari: 15.6
  npmPackages:
    @emotion/react: 11.9.3 => 11.9.3
    @emotion/styled: 11.9.3 => 11.9.3
    @mui/icons-material: 5.8.4 => 5.8.4
    @mui/lab: 5.0.0-alpha.93 => 5.0.0-alpha.93
    @mui/material: 5.9.3 => 5.9.3
    @mui/system: 5.9.3 => 5.9.3
    @mui/x-data-grid: 5.15.1 => 5.15.1
    @mui/x-data-grid-generator: 5.15.1 => 5.15.1
    @mui/x-data-grid-pro: 5.15.1 => 5.15.1
    @mui/x-date-pickers: 5.0.0-beta.4 => 5.0.0-beta.4
    @mui/x-date-pickers-pro: 5.0.0-beta.4 => 5.0.0-beta.4
    @types/react: 18.0.15 => 18.0.15
    react: 18.2.0 => 18.2.0
    react-dom: 18.2.0 => 18.2.0
    styled-components: 5.3.5 => 5.3.5
    typescript: 4.7.4 => 4.7.4

TiagoPortfolio avatar Aug 12 '22 13:08 TiagoPortfolio

I wouldn't change how it behaves, mainly because we want the buttons to have consistent height. If you disable the ripple, you would probably want to add some focus indicator around the button, so I assume you would want to have the padding. My proposal would be to add a theme override for the IconButton, that will remove the padding if disableRipple is set (or set the padding to zero if this is the only behavior you want to have).

mnajdova avatar Aug 18 '22 11:08 mnajdova

Thank you @mnajdova for your suggestion, I agree it is a better approach!

I guess I should do this:

const theme = createTheme({
  components: {
    MuiIconButton: {
      styleOverrides: {
        root: ({ ownerState }) => ({
          ...(ownerState.disableRipple && {
            p: 0,
          }),
        }),
      },
    },
  },
})

TiagoPortfolio avatar Aug 18 '22 15:08 TiagoPortfolio

Yep exactly, sorry for the late response :) Should we close the issue?

mnajdova avatar Sep 20 '22 10:09 mnajdova

Yes, thanks for the help, let's close it! :)

TiagoPortfolio avatar Sep 27 '22 11:09 TiagoPortfolio