-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiBadge] Increase color contrast of selected text (+ bonus cleanup) #7752
Conversation
+ invert those colors for `::selection` text
+ fix low contrast selection color on disabled badges - skip inline style custom colors completely if disabled, no need to calculate or apply them. this allows us to remove `!important` CSS - skip other named colors as well, also no need to apply them. and clickable styles, that definitely makes no sense
@MichaelMarcialis and @1Copenut, I think I'd like your input on this before moving forward further with this PR. It certainly accomplishes the goal of increasing selection color contrast, but I don't know if it looks... good... precisely either (see screenshots in PR description) 🙈 I'm worried the change is a little too startling to ship with as a change and I'm wondering if we should consider other approaches. Maybe using CSS color functions like https://developer.mozilla.org/en-US/docs/Web/CSS/filter-function/contrast as an alternative? |
Preview staging links for this PR:
|
💚 Build Succeeded
|
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.
@cee-chen I appreciate the ping on this one. I looked at your screenshots and live preview in light and dark themes a few times. I highlighted all badges and did spot checks for contrast ratios. All were at least 5:1, many significantly higher.
That's my objective measure. My subjective measure is, I love the look of these when text is selected. It's bold, and that's a good thing. It was immediately clear to me what text was highlighted and what was not. The high-contrast aesthetic made the UI more usable, and that's my primary concern.
I'll defer to @MichaelMarcialis for the final call if this is too bold for a first release, but for my part it's 💯 with zero qualifications.
Haha, it's fascinating how subjective visuals are! Definitely would appreciate feedback from all corners on this one! |
Thanks for the ping, @cee-chen. I largely agree with @1Copenut's assessment here. I think it's more important that the text highlighting is clear/obvious than it is to be aesthetically pleasing. That said, I think the idea of inverting the text and background colors is a simple and elegant way to achieve this. CCing the rest of the @elastic/platform-design team in case they have any thoughts as well. Forgive a potentially silly follow-up question: Since color is our only means of indicating the text highlighting, does that mean that text highlighting background colors must have a contrast ratio of 3:1 with the page background (in addition to the 4.5:1 contrast between highlight text and background colors)? If so, do we need to update our general text highlighting styles as well? |
Not a silly question at all. I think there may be some requirement for this with respect to adjacent colors. I read through SC 1.4.11: Non-text Contrast (Level AA) and there's a whole section on |
Thanks for chiming in y'all! It sounds like for the purposes of this PR we're good merging this as-is without any visual changes - am I reading that correctly? Let's continue further selection/color contrast discussions in a separate issue or possibly an actual GH discussion! |
@1Copenut Could I get an approval from you on this PR if it's good to ship? |
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.
👍 LGTM!
I added a new discussion for testing non-text contrast ratio, will post it in the relevant Slack channel.
Discussion link for completeness: #7766 |
`v94.5.0-backport.1` ⏩ `v94.5.1` [Questions? Please see our Kibana upgrade FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams) --- ## [`v94.5.1`](https://github.com/elastic/eui/releases/v94.5.1) **Bug fixes** - Fixed an `EuiDualRange`s with `showInput` bug, where `min`/`max` values and invalid states were not being correctly set if values were empty strings ([#7767](elastic/eui#7767)) **Accessibility** - Improved `EuiDatePicker` and `EuiSuperDatePicker`'s time selection screen reader UX ([#7726](elastic/eui#7726)) - Improved the accessibility of `EuiDatePicker` by providing full screen-reader-only week day names to the calendar header ([#7748](elastic/eui#7748)) - Improved `EuiBadge`'s ability to tell when text within the badge is selected/highlighted and selection color contrast ([#7752](elastic/eui#7752))
Summary
closes #7722
This PR changes the
::selection
color & background color of badges in order to make the selection more apparent to users. It also accomplishes some CSS cleanup (via utilizing CSS variables) at the same time. As always, I recommend following along by commit.QA
General checklist
::selection
on<button>
elements, this is not a bug(?)- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Checked in mobile- [ ] Added or updated jest and cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)