-
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] Panels, modals, flyouts, toasts, popovers, and tooltips #8174
[High Contrast Mode] Panels, modals, flyouts, toasts, popovers, and tooltips #8174
Conversation
8d39de5
to
3eb8a10
Compare
packages/eui/.loki/reference/chrome_mobile_Display_EuiToolTip_High_Contrast_Mode.png
Outdated
Show resolved
Hide resolved
packages/eui/src/components/popover/popover_arrow/_popover_arrow.styles.ts
Outdated
Show resolved
Hide resolved
3815597
to
53315b5
Compare
… high contrast mode - have to use actual `border` CSS and not `box-shadow` due to Windows + fix obvious components that require this - flyouts, modals, toasts
- add filter workaround for clickable panels/cards
- rewrite arrows completely using `clip-path` instead of CSS borders (required for high contrast themes, which otherwise overrides borders and as such will break on Windows) - add a pseudo element workaround for high contrast mode to preserve the border - this has some subpixel aliasing issues at different zoom levels, but is good enough IMO
- add a new story for high contrast mode, to catch regressions with the arrow rendering - fix popover footer/title screenshots to actually capture the portalled popvoer
- background color is ignored in Windows, so we have to rely entirely on borders
+ fix flakiness via `sleep`
9589b3f
to
b601612
Compare
packages/eui/src/themes/amsterdam/global_styling/mixins/shadow.ts
Outdated
Show resolved
Hide resolved
@@ -140,9 +140,17 @@ export const euiBorderColor = ( | |||
case 'subdued': | |||
return euiTheme.border.color; | |||
case 'warning': | |||
return tintOrShade(euiTheme.colors.warning, 0.4, colorMode); | |||
return tintOrShade( |
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.
Borealis note: color calculations were removed in favor of tokens. This will require consolidating.
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.
Right yeah, I think I saw that in another PR. I'll need to add a bunch more border tokens in the high contrast mode overrides/modifications
packages/eui/src/components/resizable_container/resizable_panel.styles.ts
Show resolved
Hide resolved
packages/eui/src/components/popover/popover_arrow/_popover_arrow.styles.ts
Outdated
Show resolved
Hide resolved
packages/eui/src/components/popover/popover_panel/_popover_panel.styles.ts
Outdated
Show resolved
Hide resolved
Cleanup: - remove need for pseudo element - always render a border (transparent on light mode) and update the the popover arrow position logic to offset for it - update popover positioning JS to use `100%` positioning instead of a static calculated number - update EuiToolTip to match rotation + clip-path logic (simpler all around and less chance of the arrow overlapping content)
- requires an extra wrapper due to the arrow's clip-path and transform CSS
The latest changes look good to me! |
- popover positions were flaky/imprecise - this is significantly easier to scan at a glance + update all other range VRT and improve some Storybook DX while here
5272b62
to
420570d
Compare
Thanks for the precaution! One was a pretty small pixel update but the other was pretty dang annoying, so I ended up converting to a Storybook VRT test: 8bc755b Feel free to shout if you have any Storybook preferences there. I went back and forth on making it VRT only vs not, and settled on "not", but I don't feel super strongly either way 🤷 |
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
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.
🚢 🐈⬛ Thanks for the additional changes, they look great! Awesome job on cleaning up code and tests ✨
); | ||
|
||
return { | ||
_arrowStyles: ` |
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 like the starting underscore for the base styles. Adds some more distinction between same named object and key 👍
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.
To add some more context - I tend to use underscore prefixes to denote private or internal usage. In this case I also don't want Emotion to render this extra label in its generated classNames, hence the underscore and the static string as opposed to css
template literal.
@@ -0,0 +1 @@ | |||
- Updated `EuiPopover` and `EuiToolTip` to be easier to see in dark 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.
❓ Is this fine to add here already while it's merging into a feature branch?
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.
Yes, I might either collate all the changelogs in the final feature branch PR, or I might leave them as-is. It's not bad to have the changelog pointing at the original PR so it's easier to find the atomic git history and see any discussions for color/context.
Thanks again for the fantastic review Lene, my code is so much better for it! |
Summary
Note
This PR merges into a feature branch. It continues expanding upon the high contrast mode setup (#8142), improving border display at a slightly more granular component level.
The following components with shadows should now have all-around borders in high contrast mode (both MacOS and Windows/emulated forced colors):
Panels:
hasShadow
orhasBorder
props. Split panels should not have a border between inner panels (open to feedback on the latter, but it started feeling too visually noisy at that point)Portalled components:
Popovers and tooltips:
Note for tooltips: effects are primarily only visible in dark mode or in forced colors/Windows high contrast mode mode. While I was here, I also added a border for non-high-contrast tooltips in darkmode, as I found the visibility against dark mode background colors to be insufficient to even see the dang tooltip. See the added VRT screenshots.
Popovers in particular required a good amount of custom CSS shenanigans to account for their arrows 😅 See the individual git messages for some details on the CSS tricks used to get them working (primarily using
clip-path
instead of CSS borders to render a triangle, and using a rotated pseudo element to render the border).The final effect has some minute pixel aliasing on different zoom levels, but IMO it's Good Enough and way better than just being totally broken:
Windows high contrast:
General checklist
- [ ] Checked for accessibility including keyboard-only and screenreader modes[ ] Added or updated jest and cypress tests