[Alert] Use getContrastText for filled variant font color #29813
Conversation
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We only have it in the theme.palette as part of the @mui/material's theme.
| @@ -71,7 +72,10 @@ const AlertRoot = styled(Paper, { | |||
| }), | |||
| ...(color && | |||
| ownerState.variant === 'filled' && { | |||
| color: '#fff', | |||
| color: | |||
| theme.palette.mode === 'dark' | |||
There was a problem hiding this comment.
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.
mbrookes
added
component: alert
mbrookes
changed the title
|
@SamoraMabuya Thanks for your first contribution. Can you explain the issue you are trying to fix? |
|
I was trying to get variant font color to show correct contrast color with getContrastTest method. From what I understand the test contrast is supposed to be dependent on the background color. |
#29731, but see #29813 (comment) (second sentence). |
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
I have made a small update in Alert.js
Changes:
Deploy preview: https://deploy-preview-29813--material-ui.netlify.app/components/alert/
Closes #29731