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

[High Contrast Mode] Buttons, datepicker redux, code blocks, and avatars #8210

Merged
merged 18 commits into from
Dec 10, 2024

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Dec 9, 2024

Summary

Note

This PR merges into a feature branch. I'm moving fast and bundling more disparate components into single PRs to get everything I want done in time, so as always, please follow along by commit.

The general theme of this PR was "buttons that aren't filled or empty need more contrast, get thee some borders all up in that jazz. Also let's add some borders to code blocks and avatars too while we're here".

Unfortunately QAing the button color utils led me to realize that I jumped the gun a bit with my datepicker high contrast styles in #8199, as those styles were reusing a bunch of button colors, and now they had a bunch of borders they didn't need. So yeah there's like 5 datepicker fix/cleanup PRs in there 😭 And then I figured "okay let's grab more button-adjacent components while we're here in case anything I guess", and no you're all caught up with the present day.

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked for accessibility including keyboard-only and screenreader modes
      - [ ] Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked in mobile
  • Docs site QA - N/A
  • Code quality checklist
    - [ ] Added or updated jest and cypress tests
  • Release checklist - N/A
  • Designer checklist - N/A

Sorry, something went wrong.

+ increase contrast of button group separators

+ account for window high contrast mode
…eights of various elements

- by centering with flex and static heights instead of `line-height`
- by calculating colors in date picker variables fn and passing shared date picker variables to all child styles
…eed for `serializeStlyes`

- some button colors need their preferred contrast border unset, because it looks way too noisy

+ fix time select keyboard focus causing width jumping with forced colors
- note that some colors have been deliberately simplified/changed as part of this refactor - particularly success/focus colors. IMO, this is fine and easier to manage, also I'm not even sure who's using those injected/preselected colors tbh anyway
+ switch to pseudo elements for all contrast modes - it actually fixes border-radius issues when flex wrapping

+ update VRT
- appears to be minor pixel updates / no meaningful changes
- switch from box-shadow to border so Windows high contrast mode will render it
- add transparent borders (matching EuiBadge) to all colors
- adjust line-height to accommodate borders
- we can switch to `inline-flex` over `line-height` for this component because:
  - it defaults to `vertical-align: middle` which is not as affected in differences between -flex and -block
  - it's much smaller than beta badge relative to surrounding text, which appears to affect vertical-alignment as well
- render a border (in non-forced colors, background color should be sufficient)
- force icons/SVGs to full shade to match forced color mode
@cee-chen cee-chen marked this pull request as ready for review December 9, 2024 09:54
@cee-chen cee-chen requested a review from a team as a code owner December 9, 2024 09:54
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 🐈‍⬛ I checked the updated components in high contrast mode and everything works as expected 🎉

I left a few small comments/questions but those are not blocking.

@@ -37,6 +34,10 @@ import {

export const euiDatePickerVariables = (euiThemeContext: UseEuiTheme) => {
const { euiTheme, highContrastMode } = euiThemeContext;
const unsetHighContrastBorder = <T extends object>(styles: T) => ({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opinion: While I understand the purpose of the util to prevent having to use serializeStyles , I'm wondering about the naming. Maybe I'm overcomplicating things, but this now is a kind of higher-order wrapper as it takes styles and adds additional ones.
unsetHighContrastBorder suggests a specific purpose to unset borders but it's not necessarily clear it does more (apply the passed styles), imho. Would withUnsetHighContrastBorder work better? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it's not just preventing having to use serializeStyles, it's also just way easier to stick to object notation in this file/styles since we're 1. receiving object notation from the button colors and 2. granularly reaching for individual text/bg colors elsewhere in this file.

unsetHighContrastBorder does specifically only unset the high contrast borders coming in from euiButtonColor()/euiButtonFillColor(). That's all it does. If styles is undefined, then nothing happens. No styles are being applied by default unless the variable is invoked in actual styles.

…gh contrast modes
@cee-chen cee-chen merged commit 74b2801 into elastic:high-contrast-mode Dec 10, 2024
1 of 4 checks passed
@cee-chen cee-chen deleted the high-contrast-mode-3 branch December 10, 2024 06:45
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen mentioned this pull request Dec 12, 2024
10 tasks
mgadewoll pushed a commit to mgadewoll/eui that referenced this pull request Mar 14, 2025
mgadewoll pushed a commit to mgadewoll/eui that referenced this pull request Mar 18, 2025
mgadewoll pushed a commit to mgadewoll/eui that referenced this pull request Mar 18, 2025
mgadewoll pushed a commit to mgadewoll/eui that referenced this pull request Mar 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants