search

mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.89k stars 32.26k forks source link

Button ripple not using MuiTouchRipple styles from custom theme #28551

Open howlettt opened 3 years ago

howlettt commented 3 years ago

Current Behavior 😯

Button ripple ignores MuiTouchRipple.styleOverrides in custom theme

Expected Behavior 🤔

MuiTouchRipple.styleOverride should affect the button ripple element

Steps to Reproduce 🕹

Steps:

  1. Create a custom theme
  2. Add styles under components.MuiTouchRipple.styleOverrides
  3. Add a ThemeProvider component using the custom theme
  4. Add a Button component under the ThemeProvider component

Sandbox: https://codesandbox.io/s/old-shape-fgu9g?file=/src/Demo.tsx

Context 🔦

I used the MuiTouchRipple theme style in v4 to override another style that shouldn't be applied to touch ripples

siriwatknp commented 3 years ago

Thanks for reporting the issue. From what I checked, TouchRipple is now internal component. You should override Button directly.

Feel free to close this issue if you don't have further question.

howlettt commented 3 years ago

If it's a internal component and we can't override it's style via the theme, shouldn't it be removed from the theme definition?

siriwatknp commented 3 years ago

If it's a internal component and we can't override it's style via the theme, shouldn't it be removed from the theme definition?

You are right, it should be removed from the ThemeOptions definition. Would you like to open a PR?

Samuel-Therrien-Beslogic commented 3 years ago

I was having the same issue. Please let's remove it from component style type definition to reduce confusion

siriwatknp commented 3 years ago

I was having the same issue. Please let's remove it from component style type definition to reduce confusion

This is a breaking change, so it will be removed in v6.

What we can do now is add a deprecated note to the type definition.

oliviertassinari commented 2 years ago

This is a breaking change, so it will be removed in v6.

Why is it a breaking change? As far as I understand it we don't follow semver with types and this value has no runtime impact.

siriwatknp commented 2 years ago

This is a breaking change, so it will be removed in v6.

Why is it a breaking change? As far as I understand it we don't follow semver with types and this value has no runtime impact.

The detail is here https://github.com/mui/material-ui/pull/28631.