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

[Alert] Use getContrastText for filled variant font color #29813

Merged
merged 6 commits into from Jun 22, 2022
Merged

[Alert] Use getContrastText for filled variant font color #29813

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c54e076
getContrastText method added
SamoraMabuya Nov 21, 2021
f5f0aad
Update packages/mui-material/src/Alert/Alert.js
mnajdova Jun 22, 2022
09881df
Update packages/mui-material/src/Alert/Alert.js
mnajdova Jun 22, 2022
68d4fbb
Update packages/mui-material/src/Alert/Alert.js
mnajdova Jun 22, 2022
a0aedd2
prettier
mnajdova Jun 22, 2022
a04cdbe
Merge branch 'master' into alert-filled-color
mnajdova Jun 22, 2022
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: 5 additions & 1 deletion packages/mui-material/src/Alert/Alert.js
View file
Open in desktop
Expand Up @@ -43,6 +43,7 @@ const AlertRoot = styled(Paper, {
})(({ theme, ownerState }) => {
const getColor = theme.palette.mode === 'light' ? darken : lighten;
const getBackgroundColor = theme.palette.mode === 'light' ? lighten : darken;
const getContrastText = theme.palette.mode === 'light' ? lighten : darken;
Copy link
Member

@mbrookes mbrookes Nov 21, 2021 •
edited

Choose a reason for hiding this comment

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

This returns the same as getBackgroundColor, so is redundant. It also doesn't do what its name suggests.

We have getContrastText in the theme. @mnajdova is it also in System? I couldn't find it documented for either theme or system, but I believe it's what @reramjiawan was referring to in the related issue.

Copy link
Member

Choose a reason for hiding this comment

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

We only have it in the theme.palette as part of the @mui/material's theme.

mnajdova marked this conversation as resolved.
Show resolved Hide resolved
const color = ownerState.color || ownerState.severity;

return {
Expand Down Expand Up @@ -71,7 +72,10 @@ const AlertRoot = styled(Paper, {
}),
...(color &&
ownerState.variant === 'filled' && {
color: '#fff',
color: theme.palette.getContrastText(theme.palette.mode === 'dark' ? theme.palette[color].dark : theme.palette[color].main),
theme.palette.mode === 'dark'
Copy link
Member

Choose a reason for hiding this comment

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

This (and line 46) relate to palette mode, but the value of contrast text should presumably depend on the background color being contrasted against, regardless of mode.

The standard variant already has a light background with dark text, so I'm beginning to wonder if the premise of #29731 is faulty, although I suppose it might be the case that user provided colors used in the standard variant are light enough to justify dark text.

? getContrastText(theme.palette[color].light)
: getContrastText(theme.palette[color].light, 1),
mnajdova marked this conversation as resolved.
Show resolved Hide resolved
fontWeight: theme.typography.fontWeightMedium,
backgroundColor:
theme.palette.mode === 'dark' ? theme.palette[color].dark : theme.palette[color].main,
Expand Down