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

[Chip] Fix Avatar CSS issue #18156

Merged
merged 5 commits into from
Nov 4, 2019
Merged

[Chip] Fix Avatar CSS issue #18156

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5131bea
[Chip] Force Avatar side-effect
oliviertassinari Nov 2, 2019
d778802
prefer more specificity
oliviertassinari Nov 3, 2019
6eb197c
fix visua regression, reduce specificity
oliviertassinari Nov 3, 2019
678fd1d
Update packages/material-ui/src/Chip/Chip.js
oliviertassinari Nov 3, 2019
6e01dee
yarn docs:api
oliviertassinari Nov 3, 2019
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 docs/pages/api/chip.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ Any other props supplied will be provided to the root element (native element).
| <span class="prop-name">outlinedPrimary</span> | <span class="prop-name">.MuiChip-outlinedPrimary</span> | Styles applied to the root element if `variant="outlined"` and `color="primary"`.
| <span class="prop-name">outlinedSecondary</span> | <span class="prop-name">.MuiChip-outlinedSecondary</span> | Styles applied to the root element if `variant="outlined"` and `color="secondary"`.
| <span class="prop-name">avatar</span> | <span class="prop-name">.MuiChip-avatar</span> | Styles applied to the `avatar` element.
| <span class="prop-name">avatarSmall</span> | <span class="prop-name">.MuiChip-avatarSmall</span> |
| <span class="prop-name">avatarSmall</span> | <span class="prop-name">.MuiChip-avatarSmall</span> | Styles applied to the `avatar` element is `size="small"`.
| <span class="prop-name">avatarColorPrimary</span> | <span class="prop-name">.MuiChip-avatarColorPrimary</span> | Styles applied to the `avatar` element if `color="primary"`.
| <span class="prop-name">avatarColorSecondary</span> | <span class="prop-name">.MuiChip-avatarColorSecondary</span> | Styles applied to the `avatar` element if `color="secondary"`.
| <span class="prop-name">icon</span> | <span class="prop-name">.MuiChip-icon</span> | Styles applied to the `icon` element.
Expand Down
53 changes: 29 additions & 24 deletions packages/material-ui/src/Chip/Chip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import useForkRef from '../utils/useForkRef';
import unsupportedProp from '../utils/unsupportedProp';
import capitalize from '../utils/capitalize';
import ButtonBase from '../ButtonBase';
import '../Avatar'; // So we don't have any override priority issue.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think a "harmless" side-effect such as Avatar.propTypes = Avatar.propTypes (or similar, like Avatar.a = Avatar.a) might suffice? I'm on holiday rn so I don't have much time to look into this issue myself, sorry.

Copy link
Member Author

Choose a reason for hiding this comment

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

After more reflexion, I think that having to pull out another module to get the correct side effect isn't right. It prevents tree-shaking. Autocomplete users shouldn't bundle the Avatar if they don't use 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.

Enjoy your holidays 👍


export const styles = theme => {
const backgroundColor =
Expand Down Expand Up @@ -42,6 +41,29 @@ export const styles = theme => {
opacity: 0.5,
pointerEvents: 'none',
},
'& $avatar': {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious whether or not you deem this a breaking change because strictly speaking specificity is increased 😅

Copy link
Member Author

@oliviertassinari oliviertassinari Nov 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The outlined variant already increases the specificity to style the avatar.

Copy link
Contributor

Choose a reason for hiding this comment

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

True - but let's say someone was just specifying a single avatar class name, they'd now need to also do & $avatar. Maybe not a big concern - I was more just curious to hear your perspective on breaking/non-breaking changes so we can adopt the same principle at my workplace.

Copy link
Member Author

@oliviertassinari oliviertassinari Nov 3, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@NMinhNguyen If somebody has overridden the class, he would likely face #16374, as we do. He would already be forced to increase the specificity.

marginLeft: 5,
marginRight: -6,
width: 24,
height: 24,
color: theme.palette.type === 'light' ? theme.palette.grey[700] : theme.palette.grey[300],
fontSize: theme.typography.pxToRem(12),
},
'& $avatarColorPrimary': {
color: theme.palette.primary.contrastText,
backgroundColor: theme.palette.primary.dark,
},
'& $avatarColorSecondary': {
color: theme.palette.secondary.contrastText,
backgroundColor: theme.palette.secondary.dark,
},
'& $avatarSmall': {
marginLeft: 4,
marginRight: -4,
width: 18,
height: 18,
fontSize: theme.typography.pxToRem(10),
},
},
/* Styles applied to the root element if `size="small"`. */
sizeSmall: {
Expand Down Expand Up @@ -145,32 +167,15 @@ export const styles = theme => {
backgroundColor: fade(theme.palette.secondary.main, theme.palette.action.hoverOpacity),
},
},
// TODO remove in V5
/* Styles applied to the `avatar` element. */
avatar: {
marginLeft: 5,
marginRight: -6,
width: 24,
height: 24,
color: theme.palette.type === 'light' ? theme.palette.grey[700] : theme.palette.grey[300],
fontSize: theme.typography.pxToRem(12),
},
avatarSmall: {
marginLeft: 4,
marginRight: -4,
width: 18,
height: 18,
fontSize: theme.typography.pxToRem(10),
},
avatar: {},
/* Styles applied to the `avatar` element is `size="small"`. */
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
avatarSmall: {},
/* Styles applied to the `avatar` element if `color="primary"`. */
avatarColorPrimary: {
color: theme.palette.primary.contrastText,
backgroundColor: theme.palette.primary.dark,
},
avatarColorPrimary: {},
/* Styles applied to the `avatar` element if `color="secondary"`. */
avatarColorSecondary: {
color: theme.palette.secondary.contrastText,
backgroundColor: theme.palette.secondary.dark,
},
avatarColorSecondary: {},
/* Styles applied to the `icon` element. */
icon: {
color: theme.palette.type === 'light' ? theme.palette.grey[700] : theme.palette.grey[300],
Expand Down