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

[IconButton] Add disable ripple props #15864

Merged
merged 4 commits into from May 26, 2019

Conversation

lychyi
Copy link
Contributor

@lychyi lychyi commented May 26, 2019

Fixes #15858

Also adds the same missing props to the Tab component

@lychyi lychyi marked this pull request as ready for review May 26, 2019 04:27
@mui-pr-bot
Copy link

Details of bundle changes.

Comparing: c48ee61...1ab78a1

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.04% 🔺 +0.02% 🔺 315,678 315,798 86,445 86,458
@material-ui/core/Paper 0.00% 0.00% 67,870 67,870 20,158 20,158
@material-ui/core/Paper.esm 0.00% 0.00% 61,152 61,152 18,948 18,948
@material-ui/core/Popper 0.00% 0.00% 28,740 28,740 10,346 10,346
@material-ui/core/Textarea 0.00% 0.00% 5,513 5,513 2,376 2,376
@material-ui/core/TrapFocus 0.00% 0.00% 3,744 3,744 1,575 1,575
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,960 15,960 5,781 5,781
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 975 975
@material-ui/lab 0.00% 0.00% 138,808 138,808 42,625 42,625
@material-ui/styles 0.00% 0.00% 51,353 51,353 15,176 15,176
@material-ui/system 0.00% 0.00% 14,458 14,458 4,181 4,181
Button 0.00% 0.00% 83,816 83,816 25,443 25,443
Modal 0.00% 0.00% 20,344 20,344 6,681 6,681
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 55,991 55,991 14,061 14,061
docs.main +0.01% 🔺 +0.01% 🔺 645,071 645,131 202,900 202,923
packages/material-ui/build/umd/material-ui.production.min.js +0.04% 🔺 +0.02% 🔺 294,598 294,718 83,939 83,953

Generated by 🚫 dangerJS against 1ab78a1

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Tests are ok. They are tested like in Button. We can tackle the shallow -> mount switch in another PR.

label?: React.ReactNode;
onChange?: (event: React.ChangeEvent<{ checked: boolean }>, value: any) => void;
onClick?: React.EventHandler<any>;
selected?: boolean;
style?: React.CSSProperties;
textColor?: string | 'secondary' | 'primary' | 'inherit';
value?: any;
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary for the ripple?

Copy link
Member

@leMaik leMaik May 26, 2019

Choose a reason for hiding this comment

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

It was just moved down (sorted alphabetically), it's the value prop the Tab had before.

@oliviertassinari oliviertassinari changed the title [IconButton] add disable ripple props [IconButton] Add disable ripple props May 26, 2019
@@ -37,6 +37,26 @@ describe('<Tab />', () => {
skip: ['componentProp'],
}));

it('should have a ripple by default', () => {
const wrapper = shallow(<Tab />);
Copy link
Member

Choose a reason for hiding this comment

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

We are moving away from shallow tests. Can we use the mount API?

Copy link
Member

Choose a reason for hiding this comment

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

@oliviertassinari oliviertassinari added component: icon button This is the name of the generic UI component, not the React module! new feature New feature or request labels May 26, 2019
@eps1lon eps1lon merged commit ca382ef into mui:master May 26, 2019
@eps1lon
Copy link
Member

eps1lon commented May 26, 2019

@lychyi Much appreciated. Thanks.

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! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[IconButton] Missing disableFocusRipple prop
5 participants