-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Allow palettes to specify a separate hue for uncontained text #15148
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
Comments
I've been looking at a few issues (#13905, #15672) with colored text in dark themes. Defaulting the text value to I know the PR is already merged, but I have an alternate proposal. In all the places where the If you don't agree with this change, we're going to either need a fifth hue, for separate |
Oh, and I guess cc @benelliott and @josephperrott |
@thw0rted The problem with not defaulting to 500 is that it will be a huge breaking change that invalidates a lot of screenshot diffs for apps inside Google (I know because I accidentally did it with this PR initially 😄). AFAIK because of how Angular is deployed inside Google it would then be the Material team's responsibility to go in and "fix" each of these independently. Moving forward though I definitely agree - this PR goes some way to helping the situation but it's quite easy to find situations where it still falls down. I think this will always be true however just due to the sheer number of text/background colour combinations you can end up with even within the theming engine. |
If I understand the situation correctly, the All that to say: if it continues to default to 500, How would you feel about that? I don't know what name would be appropriate, or if it's too late to change the name of the current fourth-argument key, but it sounds increasingly likely that either we need 3 keys (default, darker, lighter) or 5 (those plus "dark-tone text" and "light-tone text"). |
So, having thought about this for a while and looking at the comments in https://github.com/angular/components/blob/master/src/material/core/theming/_theming.scss#L12, I feel like an invocation of The base palette is the invariant but the This means that when you build a light or dark theme you need to create a Obviously this doesn't mean that there will be a good contrast ratio in all cases - you might still override the "dark grey" background color with a pale background color in some of your components' CSS for example. But there is no real way for the theming engine to detect that at build time. The above suggests that there should be light and dark variants of any predefined palettes in order to ensure a decent contrast ratio in all cases. So the pink Coming back to setting 500 as the default - this is purely for backwards compatibility currently. I don't disagree that 500 will be suboptimal for text contrast in a lot of (most?) cases but changing it would mean that apps inside Google break unless they start explicitly defining a text hue in their own palettes. I don't know what the appetite for such a big change would be within the Material team. Snackbar is currently a bit of an outlier because its theme colors are very hard-coded, even going as far as having an inline hex value in the theme.scss: https://github.com/angular/components/blob/master/src/material/snack-bar/_snack-bar-theme.scss#L13 This pushes me towards the idea that inverted components (like snackbar) should be themed by a separate |
I get your point about having a theme-specific palette, and it makes sense. Are you suggesting a fifth You mention Snackbar: adding an explicit |
I was actually thinking that the inverted theme would have to be a whole theme in its own right (so, a level or two up from the palette) - but still thinking about if/how this would actually work (+ without massively bloating everyone's CSS!) |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Please describe the feature you would like to request.
There is a lot of emphasis on making sure that you choose accessible text colors to ensure readability when your palette color is used in the background but there is no such concern when your palette color itself is used as the text color on a neutral background. This is exacerbated by the fact that @angular/material components will implicitly use the default hue of the palette when selecting this text color - for example in
_form-field_theme.scss
:$focused-label-color: mat-color($primary);
it's very hard to pick a palette whose primary is effective both as a strong background color and as a strong text color for small text on a neutral background and conforms to any existing brand guidelines.
One way to fix this would be to extend
mat-palette
with a fourth argument,$text
. For compatibility it could default to$default
if not specified, but this would allow you to use a much darker hue on form field labels and so on without making primary backgrounds darker everywhere else.You can quite easily see the accessibility issues with the current system with some of the prepackaged themes. For instance setting the theme to
purple-green
on https://material.angular.io/components/form-field/examples gives a contrast ratio of 2.09 for the form field inputs.What is the use-case or motivation for this proposal?
Improves accessibility
Is there anything else we should know?
No
The text was updated successfully, but these errors were encountered: