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.

Sign up for GitHub

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

Jump to bottom

[Button] Fix disableRipple getting ignored in MuiButtonBase defaultProps #29613

Closed
wants to merge 3 commits into from
Closed

[Button] Fix disableRipple getting ignored in MuiButtonBase defaultProps #29613

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b10265
test: add a failing test case for Button defaultTheme
ZeeshanTamboli Nov 10, 2021
96dae72
fix: disableRipple in theme defaultProps
ZeeshanTamboli Nov 10, 2021
aa2fc61
yarn prettier
ZeeshanTamboli Nov 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/mui-material/src/Button/Button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ const Button = React.forwardRef(function Button(inProps, ref) {
const fullWidth = fullWidthProp || fullWidthContext || false;
const size = sizeProp || sizeContext || 'medium';
const variant = variantProp || variantContext || 'text';
const disableRipple = disableRippleProp || disableRippleContext || false;
const disableRipple = disableRippleContext || disableRippleProp;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may look dubious. But if I instead do disableRippleProp || disableRippleContext, the individual buttons having disableRipple={false} do not take priority over theme's defaultProps.disableRipple. false || undefined equals undefined and defaultProps.disableRipple still wins (Reason to add the second test case).

And if we do disableRippleProp ?? disableRippleContext, the ButtonGroup behaviour will change as discussed in #28645 (comment) and would be a breaking change.

This change ensures both ButtonGroup and theme's defaultProps.disableRipple works as expected like before v5.1.0 along with keeping the use of React.Context API.
I know this is confusing, another better solution without any breaking changes would be appreciated.

Copy link
Member

@oliviertassinari oliviertassinari Nov 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't disableFocusRipple, disableElevation, disabled, fullWidth, etc. suffering from the same kind of priority issues? I don't feel like we took my comment in #28645 (comment) into account.

Copy link
Member Author

@ZeeshanTamboli ZeeshanTamboli Nov 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't disableFocusRipple, disableElevation, disabled, fullWidth, etc. suffering from the same kind of priority issues?

Not all of them has issues.
disableRipple is the only regression from v5.0.6 to 5.1.0.
disabled does not work since long with theme's defaultProps of MuiButtonBase applied to Button.
Others work since they are of MuiButton slot.

I don't feel like we took my comment in #28645 (comment) into account.

We did not consider it as more bugs but a change in behaviour resulting in it being a breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZeeshanTamboli can we add tests for all of these props and make sure they work the same?

Copy link
Member

@oliviertassinari oliviertassinari Nov 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example that demonstrates that 1. the proposed fix is not working 2. the other props are impacted https://codesandbox.io/s/basicbuttongroup-material-demo-forked-65rk8?file=/demo.js. Button 2 should have a ripple and Button 3 should not be full width.


const ownerState = {
...props,
Expand Down
41 changes: 40 additions & 1 deletion packages/mui-material/src/Button/Button.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import * as React from 'react';
import { expect } from 'chai';
import { describeConformance, act, createRenderer, fireEvent } from 'test/utils';
import { ThemeProvider, createTheme } from '@mui/material/styles';
import Button, { buttonClasses as classes } from '@mui/material/Button';
import ButtonBase from '@mui/material/ButtonBase';
import ButtonBase, { touchRippleClasses } from '@mui/material/ButtonBase';

describe('<Button />', () => {
const { render, renderToString } = createRenderer();
Expand Down Expand Up @@ -372,4 +373,42 @@ describe('<Button />', () => {

expect(container.querySelector('button')).to.have.class(disabledClassName);
});

it("should disable ripple when MuiButtonBase has disableRipple in theme's defaultProps", () => {
const theme = createTheme({
components: {
MuiButtonBase: {
defaultProps: {
disableRipple: true,
},
},
},
});
const { container } = render(
<ThemeProvider theme={theme}>
<Button>Disabled ripple</Button>
</ThemeProvider>,
);
expect(container.firstChild.querySelector(`.${touchRippleClasses.root}`)).to.equal(null);
});

it("should disable ripple when MuiButtonBase has disableRipple in theme's defaultProps but override on the individual Buttons if provided", () => {
const theme = createTheme({
components: {
MuiButtonBase: {
defaultProps: {
disableRipple: true,
},
},
},
});
const { container } = render(
<ThemeProvider theme={theme}>
<Button disableRipple={false}>Enabled ripple</Button>
<Button>Disabled ripple 1</Button>
<Button>Disabled ripple 2</Button>
</ThemeProvider>,
);
expect(container.querySelectorAll(`.${touchRippleClasses.root}`)).to.have.length(1);
});
});