-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
fix(tags): tag text color contrast #3653
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
Yea, this is where CSS variables help with specificity issues. Let me push a commit later for this 👍🏼 |
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
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.
Pushed a commit to make it more flexible to change color based on YIQ using CSS Variables without having to define modifier classes for every component.
I also change the #111
& #ddd
colors to #000
& #fff
. They easier to the eyes, the other colors looked really odd with some backgrounds.
Another thing I noticed while testing locally 🤔. Too many other colors are changing that I don't think we want them changed. That's due to the threshold of 128
which insures high contrast results. That's good for a11y but I believe we want to introduce a high contrast mode in the future for this specific case rather than change all default coloring.
Can we please increase the threshold to 138
?
Wow, what an improvement! Thank you 🚀 🚀 🚀 I need to learn about those css variables tricks.
That's fine IMO
I don't have strong opinion on this, but I'd like to test a little. Do you have examples of some unexpected color changes? |
I use tag colors from discuss, the white text color on support tag by default is fine, but isDark with the current threshold of 128 leads in dark text color. (same with 90% tags). Like I mentioned before, it's actually better for a11y for people who need really high contrast, but the intention is to fix that in the future through a high contrast mode anyway. |
Signed-off-by: Sami Mazouz <sychocouldy@gmail.com>
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.
The last commit I pushed preserves design colo consistency in both light & dark modes.
This looks ready to be merged now.
This is really cool, congrats on the long work on this!! I'm glad I'll soon be able to get rid of my CSS mess that was trying to match the color with each of my tags. |
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.
Very cool!
Fixes #3633
Depends on #3652
Changes proposed in this pull request:
Use the new
isDark
utility to add new classes (e.g.tag-dark
ortag-light
) for better contrasts between tag's bg and text.Reviewers should focus on:
My code style?
Screenshot
Necessity
Confirmed
composer test
).