[EuiProvider][EuiThemeProvider] highContrastMode set up and documentation
#8142
Conversation
cee-chen
force-pushed
the
high-contrast-mode-1
branch
3 times, most recently
from
153328b to
4e8406e
Compare
… high contrast mode detection + remove need for the render prop by fixing the global theme logic to check against defaults instead of the parent theme, which allows us to simply add a provider wrapper
+ set up context/prop types
- Prefer `modify` over computing at theme time, so that consumer modifications are also overridden and high contrast tokens are always enforced
- and set a border-bottom instead via box-shadow + code style cleanup - use optional chaining syntax instead of destructuring and correctly pass all options
+ update all provider snapshots (minor font tweaks) + remove snapshots for a story with no meaningful visuals
- note that EUI+ docs have been added by parity but will not actually work, since EUI+ points at a static EUI version/release instead of at the local workspace - update some older doc copy while here as well
+ massive cleanup/refactor of src-docs theme context, prefer toggle for light/dark mode + remove defunct tour on language selector
… ThemeContext - might as well since we're already handling localStorage things there, saves us from waterfalling props unnecessarily
cee-chen
force-pushed
the
high-contrast-mode-1
branch
from
4e8406e to
f8b0906
Compare
cee-chen
changed the title
highContrastMode set up and documentation
mgadewoll
left a comment
mgadewoll
left a comment
There was a problem hiding this comment.
Awesome work on this! 🎉 🔥
I tested:
✅ macOS increased contrast setting
✅ Windows high contrast themes
✅ emulated contrast theme on macOS in Chrome browser
Overall the code looks good to me. I left only small comments and a general question about how we might want to support testing via the theme switcher.
packages/eui/src-docs/src/components/guide_theme_selector/guide_theme_selector.tsx
Show resolved
Hide resolved
| const modificationsWithHighContrast = useMemo(() => { | ||
| if (!highContrastMode) return modifications; | ||
|
|
||
| const borderColor = system.root.colors[colorMode].fullShade; |
There was a problem hiding this comment.
🗒️ Note to self: Here we'll need to account for the new tokens in Borealis when merging to the feature branch.
There was a problem hiding this comment.
I'll probably end up moving all those tokens to a separate file/hook util once that happens, since there will be a lot more overrides and I don't want to add that much context to the main provider file.
Actually, let me just go ahead and do that now while I'm here 🧠
There was a problem hiding this comment.
Seperate hook util added in 1e79385
This feels much cleaner already! Hooray for sweeping my excessively long code spaghet comments into a corner where no one will see them 🤣
There was a problem hiding this comment.
Although, hm, now I'm wondering if I should have moved the hook to utils.ts instead of creating a brand new file. utils.ts already feels plenty long and scary though... any preference?
There was a problem hiding this comment.
I think it's nice to have a dedicated file for the high contrast overrides. It ensures a clear scope and prevents additional file bloat, utils.ts is already quite long and scary indeed 👍
There was a problem hiding this comment.
Sounds good! Thanks for sanity checking me as always Lene! ✨
packages/eui/src-docs/src/views/theme/high_contrast_mode/high_contrast_mode_example.js
Outdated
Show resolved
Hide resolved
packages/eui/src-docs/src/views/theme/high_contrast_mode/high_contrast_mode_example.js
Outdated
Show resolved
Hide resolved
| context?: any; | ||
| }; | ||
|
|
||
| export const GuideThemeSelector: React.FunctionComponent< |
There was a problem hiding this comment.
🗒️ General note: This will cause some merge/rebase conflicts when merging to the Borealis feature branch as this was implemented there too.
+ generally improve story DX for EuiProvider
…anticipation of new theme tokens - there'll be a lot more, so let's pull it out to its own file and keep the main provider fairly high level in terms of high contrast mode logic
cee-chen
force-pushed
the
high-contrast-mode-1
branch
from
cf38eee to
1e79385
Compare
|
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
packages/eui/src-docs/src/components/guide_theme_selector/guide_theme_selector.tsx
Show resolved
Hide resolved
mgadewoll
left a comment
mgadewoll
left a comment
There was a problem hiding this comment.
🚢 🐈⬛ Thanks for the additional updates!
I left one more non-blocking comment. Feel free to ignore if you think it's fine as is.
4 participants
Summary
Note
This PR merges into a feature branch. As this PR contains some heavy docs changes, I recommend following along by commit.
This PR sets up the initial architecture for a
highContrastModeprop/return in<EuiThemeProvider>anduseEuiTheme(). By default, this setting will inherit from the user's system settings with one of 3 possible options:"forced"(Windows high contrast mode and some browser forced colors extensions)"preferred"(MacOS high contrast mode)false(high contrast mode not on)The provider prop takes a simple true/false (aka
preferredorfalse-forcedis something that can only be set externally by the user/system and not by the consumer/application), whereas the internal state returned by our hooks returns more the granular enum for help with styling and other considerations.What this PR does not do, and will be handled in future PRs:
QA
General checklist
- [ ] Checked in mobile- [ ] Checked for accessibility including keyboard-only and screenreader modes@defaultif default values are missing)and playground toggles- [ ] Checked Code Sandbox works for any docs examplesand cypress tests- [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)