-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[Button] Fix secondary contrastText color #9913
[Button] Fix secondary contrastText color #9913
Conversation
@ValentinH Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert.
@@ -119,7 +119,7 @@ export const styles = theme => ({ | |||
}, | |||
}, | |||
raisedAccent: { | |||
color: theme.palette.getContrastText(theme.palette.secondary.light), | |||
color: theme.palette.secondary.contrastText, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is applying the contrastText
color calculated for the secondary.main
color to a light background. If the contrastText
color is white, it's going to have a low contrast ratio against secondary.light
. It also introduces an inconsistency with other components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbrookes Yes, it's the next topic I want to fix. We should be using main and dark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I was also going to submit an issue about why the default raisedAccent is not the main one and the hover the dark one. Is it what you are talking about @oliviertassinari
Sorry, I don't know what I'm supposed to be looking at with those links, or what the "next" topic is. For this topic, unfortunatety this PR introduces a regression. |
@mbrookes I'm on it. |
As discussed in #9908