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

[styles] Fix resolution of default props #24253

Merged
merged 4 commits into from Jan 9, 2021
Merged

[styles] Fix resolution of default props #24253

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1b9752
[styles] Fix resolution of default props
oliviertassinari Jan 3, 2021
2891a3f
consistency
oliviertassinari Jan 3, 2021
a80cc7d
add test case
oliviertassinari Jan 4, 2021
03aca94
test the other components
oliviertassinari Jan 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/material-ui/src/Avatar/Avatar.js
View file
Open in desktop
Expand Up @@ -8,11 +8,11 @@ import Person from '../internal/svg-icons/Person';
import avatarClasses, { getAvatarUtilityClass } from './avatarClasses';

const overridesResolver = (props, styles) => {
const { colorDefault, variant = 'circular' } = props;
const { styleProps } = props;

return deepmerge(styles.root || {}, {
...styles[variant],
...(colorDefault && styles.colorDefault),
...styles[styleProps.variant],
...(styleProps.colorDefault && styles.colorDefault),
[`& .${avatarClasses.img}`]: styles.img,
[`& .${avatarClasses.fallback}`]: styles.fallback,
});
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/src/Avatar/Avatar.test.js
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@ describe('<Avatar />', () => {
muiName: 'MuiAvatar',
testDeepOverrides: { slotName: 'fallback', slotClassName: classes.fallback },
testVariantProps: { variant: 'foo' },
testStateOverrides: { prop: 'variant', value: 'rounded', styleKey: 'rounded' },
skip: ['componentsProp'],
}));

Expand Down
23 changes: 7 additions & 16 deletions packages/material-ui/src/Badge/Badge.js
View file
Open in desktop
Expand Up @@ -24,28 +24,19 @@ const RADIUS_STANDARD = 10;
const RADIUS_DOT = 4;

const overridesResolver = (props, styles) => {
const {
color = 'default',
variant = 'standard',
anchorOrigin = {
vertical: 'top',
horizontal: 'right',
},
invisible,
overlap = 'rectangular',
} = props;
const { styleProps } = props;

return deepmerge(styles.root || {}, {
[`& .${badgeClasses.badge}`]: {
...styles.badge,
...styles[variant],
...styles[styleProps.variant],
...styles[
`anchorOrigin${capitalize(anchorOrigin.vertical)}${capitalize(
anchorOrigin.horizontal,
)}${capitalize(overlap)}`
`anchorOrigin${capitalize(styleProps.anchorOrigin.vertical)}${capitalize(
styleProps.anchorOrigin.horizontal,
)}${capitalize(styleProps.overlap)}`
],
...(color !== 'default' && styles[`color${capitalize(color)}`]),
...(invisible && styles.invisible),
...(styleProps.color !== 'default' && styles[`color${capitalize(styleProps.color)}`]),
...(styleProps.invisible && styles.invisible),
},
});
};
Expand Down
26 changes: 10 additions & 16 deletions packages/material-ui/src/Button/Button.js
View file
Open in desktop
Expand Up @@ -10,30 +10,24 @@ import capitalize from '../utils/capitalize';
import buttonClasses, { getButtonUtilityClass } from './buttonClasses';

const overridesResolver = (props, styles) => {
const {
color = 'primary',
disableElevation = false,
fullWidth = false,
size = 'medium',
variant = 'text',
} = props;
const { styleProps } = props;

return deepmerge(styles.root || {}, {
...styles[variant],
...styles[`${variant}${capitalize(color)}`],
...styles[`size${capitalize(size)}`],
...styles[`${variant}Size${capitalize(size)}`],
...(color === 'inherit' && styles.colorInherit),
...(disableElevation && styles.disableElevation),
...(fullWidth && styles.fullWidth),
...styles[styleProps.variant],
...styles[`${styleProps.variant}${capitalize(styleProps.color)}`],
...styles[`size${capitalize(styleProps.size)}`],
...styles[`${styleProps.variant}Size${capitalize(styleProps.size)}`],
...(styleProps.color === 'inherit' && styles.colorInherit),
...(styleProps.disableElevation && styles.disableElevation),
...(styleProps.fullWidth && styles.fullWidth),
[`& .${buttonClasses.label}`]: styles.label,
[`& .${buttonClasses.startIcon}`]: {
...styles.startIcon,
...styles[`iconSize${capitalize(size)}`],
...styles[`iconSize${capitalize(styleProps.size)}`],
},
[`& .${buttonClasses.endIcon}`]: {
...styles.endIcon,
...styles[`iconSize${capitalize(size)}`],
...styles[`iconSize${capitalize(styleProps.size)}`],
},
});
};
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/src/Button/Button.test.js
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@ describe('<Button />', () => {
muiName: 'MuiButton',
testDeepOverrides: { slotName: 'label', slotClassName: classes.label },
testVariantProps: { variant: 'contained', fullWidth: true },
testStateOverrides: { prop: 'size', value: 'small', styleKey: 'sizeSmall' },
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
skip: ['componentsProp'],
}));

Expand Down
6 changes: 3 additions & 3 deletions packages/material-ui/src/ButtonBase/ButtonBase.js
View file
Open in desktop
Expand Up @@ -11,11 +11,11 @@ import TouchRipple from './TouchRipple';
import buttonBaseClasses from './buttonBaseClasses';

const overridesResolver = (props, styles) => {
const { disabled, focusVisible } = props;
const { styleProps } = props;

return deepmerge(styles.root || {}, {
...(disabled && styles.disabled),
...(focusVisible && styles.focusVisible),
...(styleProps.disabled && styles.disabled),
...(styleProps.focusVisible && styles.focusVisible),
});
};

Expand Down
32 changes: 13 additions & 19 deletions packages/material-ui/src/Slider/Slider.js
View file
Open in desktop
Expand Up @@ -26,40 +26,34 @@ export const sliderClasses = {
};

const overridesResolver = (props, styles) => {
const {
color = 'primary',
marks: marksProp = false,
max = 100,
min = 0,
orientation = 'horizontal',
step = 1,
track = 'normal',
} = props;
const { styleProps } = props;

const marks =
marksProp === true && step !== null
? [...Array(Math.floor((max - min) / step) + 1)].map((_, index) => ({
value: min + step * index,
}))
: marksProp || [];
styleProps.marksProp === true && styleProps.step !== null
? [...Array(Math.floor((styleProps.max - styleProps.min) / styleProps.step) + 1)].map(
(_, index) => ({
value: styleProps.min + styleProps.step * index,
}),
)
: styleProps.marksProp || [];

const marked = marks.length > 0 && marks.some((mark) => mark.label);

return deepmerge(styles.root || {}, {
...styles[`color${capitalize(color)}`],
...styles[`color${capitalize(styleProps.color)}`],
[`&.${sliderClasses.disabled}`]: styles.disabled,
...(marked && styles.marked),
...(orientation === 'vertical' && styles.vertical),
...(track === 'inverted' && styles.trackInverted),
...(track === false && styles.trackFalse),
...(styleProps.orientation === 'vertical' && styles.vertical),
...(styleProps.track === 'inverted' && styles.trackInverted),
...(styleProps.track === false && styles.trackFalse),
[`& .${sliderClasses.rail}`]: styles.rail,
[`& .${sliderClasses.track}`]: styles.track,
[`& .${sliderClasses.mark}`]: styles.mark,
[`& .${sliderClasses.markLabel}`]: styles.markLabel,
[`& .${sliderClasses.valueLabel}`]: styles.valueLabel,
[`& .${sliderClasses.thumb}`]: {
...styles.thumb,
...styles[`thumbColor${capitalize(color)}`],
...styles[`thumbColor${capitalize(styleProps.color)}`],
[`&.${sliderClasses.disabled}`]: styles.disabled,
},
});
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/src/Slider/Slider.test.js
View file
Open in desktop
Expand Up @@ -39,6 +39,7 @@ describe('<Slider />', () => {
muiName: 'MuiSlider',
testDeepOverrides: { slotName: 'thumb', slotClassName: classes.thumb },
testVariantProps: { color: 'primary', orientation: 'vertical' },
testStateOverrides: { prop: 'color', value: 'secondary', styleKey: 'colorSecondary' },
}));

it('should call handlers', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/material-ui/src/Typography/Typography.js
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@ const getTextColor = (color, palette) => {
};

const overridesResolver = (props, styles) => {
const { styleProps = {} } = props;
const { styleProps } = props;

return deepmerge(styles.root || {}, {
...(styleProps.variant && styles[styleProps.variant]),
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui/src/Typography/Typography.test.js
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@ describe('<Typography />', () => {
refInstanceof: window.HTMLParagraphElement,
muiName: 'MuiTypography',
testVariantProps: { color: 'secondary', variant: 'dot' },
testStateOverrides: { prop: 'variant', value: 'h2', styleKey: 'h2' },
skip: ['componentsProp'],
}));

Expand Down
33 changes: 32 additions & 1 deletion test/utils/describeConformanceV5.js
View file
Open in desktop
Expand Up @@ -57,7 +57,38 @@ function testThemeComponents(element, getOptions) {
expect(container.firstChild).to.have.attribute(testProp, 'testProp');
});

it("respect theme's styleOverrides", () => {
it("respect theme's styleOverrides custom state", () => {
const { muiName, testStateOverrides } = getOptions();

if (!testStateOverrides) {
return;
}
Comment on lines +63 to +65
Copy link
Member

Choose a reason for hiding this comment

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

When should this not be tested?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you ask me, I would say most of the time we don't need to test it. We don't even fully test it right now, we take an arbitrary case. I would personally prefer we use the Button or Slider as a stress test template. It should be enough.

Badge is still untested.

The badge doesn't have state on its root element.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have moved away from the API in #24095 with this patch https://gist.github.com/oliviertassinari/60614c8c37d4ba10adc2a3b1aba1277e. The fix is meant for backward compatibility with v4.

Copy link
Member

@eps1lon eps1lon Jan 5, 2021 •
edited

Choose a reason for hiding this comment

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

Badge is still untested.

The badge doesn't have state on its root element.

Then why did we need to change the implementation?

If you ask me, I would say most of the time we don't need to test it.

It already broke once.

The fix is meant for backward compatibility with v4.

describeConformanceV5 is not meant for v4.

Copy link
Member Author

Choose a reason for hiding this comment

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

It already broke once.

Exactly why I suggest the current tradeoff. If it breaks a second time, it will be a signal that it isn't enough.

Copy link
Member

Choose a reason for hiding this comment

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

The argument I have tried to defend is that it doesn't matter.

It does matter. Every implementation needs a test unless it isn't testable at which point one needs to make an argument why it isn't testable.

We don't decide by who writes the code if it needs to be tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

Every implementation needs a test unless it isn't testable

I respectfully disagree. I think that we should also add "unless it's not worth it".

I think that it's like the 100% code coverage I have tried to push in the past and we gave up on. It's not always worth the time, once the opportunity cost is high, best to defer and be lazy.

Copy link
Member

@eps1lon eps1lon Jan 8, 2021 •
edited

Choose a reason for hiding this comment

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

I respectfully disagree. I think that we should also add "unless it's not worth it".

Impossible to collaborate on that premise since this argument applies anytime. Also: please stop cutting the quote. This is not what I said.

I think that it's like the 100% code coverage I have tried to push in the past and we gave up on.

At some point there comes a time where you should educate yourself more on the topic. I can only bring nudge you so far but if you believe that line coverage has anything to do with behavior testing I don't think you put any thought into verifying your work.

It becomes evident to me that you don't know at all why you made this change. I'm asking you a third time now: Why was the Badge changed but no test added? If you don't know what the code does then please revert it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Impossible to collaborate on that premise since this argument applies anytime.

I think that we can default to testing less when we have a doubt. We have done this many times e.g. bugs in IE11, not worth it, it has been more efficient to have regressions and handle them. I think that it's best to wait for the pain to grow before doing something about it, we already have so many problems we know are important and need care.

Why was the Badge changed but no test added?

I have tried to answer it in #24253 (comment), the new generic test inside describeConformanceV5 doesn't support it. It's one rounding error compared to all the other components (144). Best to ignore it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry but you have the responsibility to explain the code you write. We have this expectation for any other contribution. What you did, expected behavior, actual behavior applies to everyone.


const testStyle = {
marginTop: '13px',
};

const theme = createMuiTheme({
components: {
[muiName]: {
styleOverrides: {
[testStateOverrides.styleKey]: testStyle,
},
},
},
});

const { container } = render(
<ThemeProvider theme={theme}>
{React.cloneElement(element, {
[testStateOverrides.prop]: testStateOverrides.value,
})}
</ThemeProvider>,
);
expect(container.firstChild).to.toHaveComputedStyle(testStyle);
});

it("respect theme's styleOverrides slots", () => {
const { muiName, testDeepOverrides } = getOptions();

const testStyle = {
Expand Down