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

[EuiPopover] Increase visibility in dark mode #7310

Merged
merged 5 commits into from
Oct 30, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Oct 24, 2023

Summary

closes #5788

This PR very slightly (.25%) tints the empty shade background of EuiPopovers in dark mode to increase visibility / contrast against existing page/panel backgrounds.

Unlike Elizabet's suggestion in #5788 (comment), we cannot simply change the background from emptyShade to lightShade - this completely blows out borders and other elements that use lightShade (see below screenshot of the border between "dark" and the switch).

Before After

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes color contrast
      - [ ] Checked in mobile
  • Docs site QA - N/A
  • Code quality checklist - N/A
  • 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
    • Updated the Figma library counterpart

@cee-chen cee-chen force-pushed the popover/dark-mode-color branch from 2f19a06 to 03f8f15 Compare October 24, 2023 17:49
@cee-chen cee-chen changed the title [EuiPopover] Increase visibility/contrast in dark mode [EuiPopover] Increase visibility in dark mode Oct 24, 2023
@cee-chen cee-chen marked this pull request as ready for review October 24, 2023 19:33
@cee-chen cee-chen requested a review from a team as a code owner October 24, 2023 19:33
we're now overriding it via `background-color`, and it doesn't work to color the popover arrow in any case
@cee-chen
Copy link
Contributor Author

@MichaelMarcialis @andreadelrio @ryankeairns @kyrspl (pinging multiple designers since we don't have one assigned to EUI) - I'd appreciate your sanity check and feedback on this PR, in particular if any of you have any blocking concerns about this change.

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

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! Verified the difference in background color in four evergreen browsers.

@cee-chen
Copy link
Contributor Author

Thanks Trevor! Hoping to hear from at least one designer before merge. If I don't hear anything or no one vetoes or makes any change requests, I'll merge this PR on Friday.

@ryankeairns ryankeairns self-requested a review October 25, 2023 18:20
@mdefazio
Copy link
Contributor

Sneaking in here...but I like it!

@kyrspl
Copy link
Contributor

kyrspl commented Oct 26, 2023

I like the idea of playing with the tint level. This is also described in Material 2.
Do we have other situations where this would apply to which we’ll need to take into account? I’m thinking: Cards inside flyouts or modals, dashboards etc?

Material 3 has an interesting take on this issue: Their newest approach focuses on a robust colour system that emphasizes separation through surface colors. Looking at the previous version of Material, hierarchy of depth was primarily established through elevation (i.e. shadows). They describe how to use light to solve the issue we experience here: “The higher a surface's elevation (raising it closer to an implied light source), the lighter that surface becomes.”

In contrast (no pun intended), the newer version leans heavily on specific surface colours to define contained areas and suggests to use shadows only when required (e.g. suggesting an interaction).

In light of the above (again, no pun intended), it’s worth exploring these ideas when looking at a new theme.

@ryankeairns
Copy link
Contributor

I like the idea of playing with the tint level. This is also described in Material 2. Do we have other situations where this would apply to which we’ll need to take into account? I’m thinking: Cards inside flyouts or modals, dashboards etc?

Material 3 has an interesting take on this issue: Their newest approach focuses on a robust colour system that emphasizes separation through surface colors. Looking at the previous version of Material, hierarchy of depth was primarily established through elevation (i.e. shadows). They describe how to use light to solve the issue we experience here: “The higher a surface's elevation (raising it closer to an implied light source), the lighter that surface becomes.”

In contrast (no pun intended), the newer version leans heavily on specific surface colours to define contained areas and suggests to use shadows only when required (e.g. suggesting an interaction).

In light of the above (again, no pun intended), it’s worth exploring these ideas when looking at a new theme.

Yes, I'm confident there are other spots. Using dark mode myself, this is quite evident - that it could use some shading tweaks as it all sort of runs together, imo. Likely a good future exercise to comb through and/or test this layer:shading approach... separate from this PR :)

@cee-chen cee-chen merged commit cfa84f4 into elastic:main Oct 30, 2023
1 check passed
@cee-chen cee-chen deleted the popover/dark-mode-color branch October 30, 2023 19:26
@cee-chen cee-chen mentioned this pull request Nov 27, 2023
jbudz pushed a commit to elastic/kibana that referenced this pull request Dec 18, 2023
`v90.0.0`⏩`v91.0.0-backport.0`

⚠️ While this upgrade pings many teams and has a large code diff, **the
majority of the changes are snapshots or tests-related** and do not
touch source code, so should theoretically only need a code review and
not dedicated QA.

The changes in EUI that required a large swathe of these updates are:

- **EuiPopover** removed an extra unnecessary `<div>` wrapper on its
anchors, which affected many snapshots and a few CSS overrides, which
should have been updated
- **EuiButtonGroup** now renders `<button>` elements instead of `<input
type="radio">` elements for single selection, which affected both
snapshots and E2E tests
- **EuiSuperDatePicker**'s absolute date input now requires an `Enter`
keypress when parsing dates (affected E2E tests)
- **EuiComboBox**, when rendered with `singleSelection={{ plainText:
'true' }}`, no longer renders a pill (i.e. text). This combobox type now
behaves more like an `EuiFieldText`, where the selection is rendered via
input `value` instead. This affected a high amount of E2E tests (both
FTR and Cypress), both in terms of updating assertions and changing
selections, but should **not** significantly affect user experience -
see elastic/eui#7332 for more.

---

##
[`v91.0.0-backport.0`](https://github.com/elastic/eui/tree/v91.0.0-backport.0)

**This is a backport release only intended for use by Kibana.**

- Added `esqlVis`, `pipeBreaks`, and `pipeNoBreaks` icon glyphs.
- `EuiSelectable` now allows configurable text truncation via
`listProps.truncationProps`
([#7388](elastic/eui#7388))
- `EuiTextTruncate` now supports a new `calculationDelayMs` prop for
working around font loading or layout shifting scenarios
([#7388](elastic/eui#7388))

**Bug fixes**

- Fixed a bug with `EuiSelectable`s with custom `truncationProps`, where
scrollbar widths were not being accounted for
([#7392](elastic/eui#7392))

## [`91.0.0`](https://github.com/elastic/eui/tree/v91.0.0)

- Updated the background color of `EuiPopover`s in dark mode to increase
visibility & contrast against other page/panel backgrounds
([#7310](elastic/eui#7310))
- Memoized `EuiDataGrid` to prevent unneeded re-renders
([#7324](elastic/eui#7324))
- Added a configurable `role` prop to `EuiAccordion`
([#7326](elastic/eui#7326))
- Added a configurable `role` prop to `EuiGlobalToastList`
([#7328](elastic/eui#7328))
- For greater flexibility, `EuiSuperDatePicker` now allows users to
paste ISO 8601, RFC 2822, and Unix timestamps in the `Absolute` tab
input, in addition to timestamps in the `dateFormat` prop
([#7331](elastic/eui#7331))
- Plain text `EuiComboBox`es now behave more like a normal text
field/input. Backspacing will no longer delete the entire value, and
selected values can now be double clicked and copied.
([#7332](elastic/eui#7332))
- `EuiDataGrid`'s display settings popover now allows users to clear the
"Lines per row" input before typing in a new number
([#7338](elastic/eui#7338))
- Improved the UX of `EuiSuperDatePicker`'s Absolute tab for users
manually typing in timestamps
([#7341](elastic/eui#7341))
- Updated `EuiI18n`s with multiple `tokens` to accept dynamic `values`
([#7341](elastic/eui#7341))

**Bug fixes**

- Fixed `EuiComboBox`'s `onSearchChange` callback to pass the correct
`hasMatchingOptions` value
([#7334](elastic/eui#7334))
- Fixed an `EuiSelectableTemplateSitewide` bug where the `popoverButton`
behavior would break if passed a non-DOM React wrapper
([#7339](elastic/eui#7339))

**Deprecations**

- `EuiPopover`: deprecated `anchorClassName`. Use `className` instead
([#7311](elastic/eui#7311))
- `EuiPopover`: deprecated `buttonRef`. Use `popoverRef` instead
([#7311](elastic/eui#7311))
- `EuiPopover`: removed extra `.euiPopover__anchor` div wrapper. Target
`.euiPopover` instead if necessary
([#7311](elastic/eui#7311))
- Deprecated `EuiButtonGroup`'s `name` prop. This can safely be removed.
([#7325](elastic/eui#7325))

**Breaking changes**

- Removed deprecated `euiPaletteComplimentary` - use
`euiPaletteComplementary` Instead
([#7333](elastic/eui#7333))

**Accessibility**

- Updated `type="single"` `EuiButtonGroup`s to render standard buttons
instead of radio buttons under the hood, per recent a11y recommendations
([#7325](elastic/eui#7325))
- `EuiAccordion` now defaults to a less screenreader-noisy `group` role
instead of `region`. If your accordion contains significant enough
content to be a document landmark role, you may re-configure it back to
`region`. ([#7326](elastic/eui#7326))
- Reduced screen reader noisiness when sorting `EuiDataGrid` columns via
toolbar ([#7327](elastic/eui#7327))
- `EuiGlobalToastList` now defaults to a `log` role. If your toasts will
always require immediate user action, consider (with caution) using the
`alert` role instead.
([#7328](elastic/eui#7328))

**CSS-in-JS conversions**

- Updated `$euiFontFamily` and `$euiCodeFontFamily` to match Emotion
fonts ([#7332](elastic/eui#7332))

---------

Co-authored-by: Cee Chen <constance.chen@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
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.

[EuiPopover] Background in dark mode isn't different from the main background
7 participants