-
Notifications
You must be signed in to change notification settings - Fork 843
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
[High Contrast Mode] Buttons, datepicker redux, code blocks, and avatars #8210
Conversation
+ 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
+ account for windows high contrast
- render a border (in non-forced colors, background color should be sufficient) - force icons/SVGs to full shade to match forced color mode
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.
🚢 🐈⬛ 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.
packages/eui/.loki/reference/chrome_mobile_Navigation_EuiButtonGroup_High_Contrast.png
Outdated
Show resolved
Hide resolved
@@ -37,6 +34,10 @@ import { | |||
|
|||
export const euiDatePickerVariables = (euiThemeContext: UseEuiTheme) => { | |||
const { euiTheme, highContrastMode } = euiThemeContext; | |||
const unsetHighContrastBorder = <T extends object>(styles: T) => ({ |
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.
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? 🤔
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.
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
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
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
and screenreader modes- [ ] Checked in Chrome, Safari, Edge, and Firefox- [ ] Checked in mobile- [ ] Added or updated jest and cypress tests