Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[material-ui][IconButton] disableRipple on MuiButtonBase doesn't disable ripple effect on IconButton #43617

Closed
Studio384 opened this issue Sep 5, 2024 · 4 comments · Fixed by #43714
Assignees
Labels
component: icon button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material regression A bug, but worse

Comments

@Studio384
Copy link
Contributor

Studio384 commented Sep 5, 2024

Steps to reproduce

Link to live example: https://codesandbox.io/p/sandbox/friendly-sunset-w3d9ft

Steps:

  1. Create a theme that disables the ripple effect globally through MuiButtonBase as described in the documentation.
  2. Add IconButton components.
  3. Also create a theme that disables the ripple effect directly on MuiIconButton.
  4. Add IconButton components here too.

Current behavior

  1. In the first example, the ripple effect won't be disabled on IconButton components. The prop is completely ignored, where in v5 this would disable the ripple.
  2. For the second example, now not only is the ripple disabled, but so is the hover effect.

Expected behavior

  1. Disabling ripple on MuiButtonBase disables it for all button types, as v5 used to do.
  2. Disabling ripple on MuiIconButton does not disable any hover effects, as this property does when applied to MuiButtonBase. This issue is also present in v5.

Context

No response

Your environment

No response

Search keywords: ripple icon button

@Studio384 Studio384 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 5, 2024
@zannager zannager added component: icon button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Sep 5, 2024
@sai6855
Copy link
Contributor

sai6855 commented Sep 5, 2024

Looks like this regression is from #43271

This is how disabledRipple got resolved in v5

check for disabledRipple prop in IconButton (if not defined) -> check for MuiIconButton theme default props (if not defined), check for MuiButtonBase theme default props.

But this behaviour was changed here, disabledRipple was explicitly set to false if it's not defined at step 2, so check couldn't go for step 3.

I think we can solve this like below, but i'm not sure how efficient solution is

const IconButton = React.forwardRef(function IconButton(inProps, ref) {
  const props = useDefaultProps({ props: inProps, name: 'MuiIconButton' });
+  // eslint-disable-next-line material-ui/mui-name-matches-component-name
+  const buttonBaseProps = useDefaultProps({ props: inProps, name: 'MuiButtonBase' });
  const {
    edge = false,
    children,
    className,
    color = 'default',
    disabled = false,
 disableFocusRipple = false,
- disableRipple = false,
+    disableRipple: disableRippleProp,
    size = 'medium',
    ...other
  } = props;

+  const disableRipple = disableRippleProp ?? buttonBaseProps.disableRipple ?? false;

@siriwatknp siriwatknp added regression A bug, but worse and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 11, 2024
@siriwatknp siriwatknp moved this to Backlog in Material UI Sep 11, 2024
@siriwatknp
Copy link
Member

siriwatknp commented Sep 11, 2024

@sai6855 Let's revert the change in #43271 and fix the variants of IconButton to:

variants: [
      {
-        props: { disableRipple: false },
+       props: (props) => !props.disableRipple,
        style: {
          '&:hover': {
            backgroundColor: theme.vars
              ? `rgba(${theme.vars.palette.action.activeChannel} / ${theme.vars.palette.action.hoverOpacity})`
              : alpha(theme.palette.action.active, theme.palette.action.hoverOpacity),
            // Reset on touch devices, it doesn't add specificity
            '@media (hover: none)': {
              backgroundColor: 'transparent',
            },
          },
        },
      },

Copy link

github-actions bot commented Oct 9, 2024

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

@Studio384 How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

@DiegoAndai
Copy link
Member

The regression portion of this issue is solved in #43714.

About the issue that is also present in v5: "Disabling ripple on MuiIconButton does not disable any hover effects, as this property does when applied to MuiButtonBase." @Studio384 may I ask you to create a separate issue for it so it's easier to track?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: icon button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material regression A bug, but worse
Projects
Status: Done
5 participants