Skip to content
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

Merged
merged 4 commits into from
May 16, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented May 13, 2024

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.

Light mode Dark mode

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in Chrome, Safari, Edge, and Firefox - NOTE: Firefox does not show ::selection on <button> elements, this is not a bug(?)
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
      - [ ] Checked in mobile
  • Docs site QA - N/A
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist - Skipped, not sure selection is something we've ever documented in Figma

+ 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
@cee-chen
Copy link
Member Author

@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?

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@cee-chen cee-chen marked this pull request as ready for review May 14, 2024 15:06
@cee-chen cee-chen requested a review from a team as a code owner May 14, 2024 15:06
Copy link
Contributor

@1Copenut 1Copenut left a 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.

@cee-chen
Copy link
Member Author

Haha, it's fascinating how subjective visuals are! Definitely would appreciate feedback from all corners on this one!

@MichaelMarcialis
Copy link
Contributor

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?

@1Copenut
Copy link
Contributor

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 Adjacent Colors and the 3:1 contrast requirement. The example uses a text input compared to the background color around it, but there's probably a case for background color to selected (highlighted) background color too.

@cee-chen
Copy link
Member Author

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!

@cee-chen
Copy link
Member Author

@1Copenut Could I get an approval from you on this PR if it's good to ship?

Copy link
Contributor

@1Copenut 1Copenut left a 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.

@cee-chen
Copy link
Member Author

Discussion link for completeness: #7766

@cee-chen cee-chen merged commit 1dd68eb into elastic:main May 16, 2024
6 checks passed
@cee-chen cee-chen deleted the badge/colors branch May 16, 2024 18:50
mgadewoll added a commit to elastic/kibana that referenced this pull request May 22, 2024
`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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants