Skip to content

[Theme] Support theme level overrides #8558

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mgadewoll
Copy link
Contributor

@mgadewoll mgadewoll commented Apr 7, 2025

Summary

Closes https://github.com/elastic/eui-private/issues/281

This PR adds support for theme-specific overrides directly on the theme definition layer.
While we already have ways to override theme tokens (using the modify prop on the EuiProvider) this way is limited to what's available to the provider (e.g. it does not have access to the primitive tokens).

Note

HCM: Currently we add overrides for high contrast mode here.
This use passes overrides via the modify prop to the provider. Anything added there currently is applied to any theme active on the provider, it's a global override.

For theme-specific overrides we instead would want to provide the overrides directly on the definition layer (e.g. here).
A use case we'll need to support this way soon: specific sets of data vis colors in high contrast mode.

The suggested change enables the current theming functionality to support an optional overrides key on the EuiThemeShape and updates the getComputed theming function to include the required overrides (the implementation so far considers HCM (high contrast mode) overrides. The approach follows what's already available for defining LIGHT and DARK variants per color mode. The overrides key is part of the EuiThemeShape (input theme) only, it will not be included on the EuiThemeComputed (output theme).

usage
Screenshot 2025-04-07 at 16 06 51

Note

The code for building and updating themes is old, not that easy to follow and overall rather cumbersome. This PR only adds onto the code for now. In the future (likely once we have fully migrated off Amsterdam and finished Borealis) we might want to consider refactoring those functions in general to improve functionality and maintainability.

Changes

  • updates getComputed to support highContrastMode via overrides.HCM on the theme object
  • updates inherited color tokens to use computed values (where possible) to ensure tokens are updated accordingly by overrides they inherit from (the alternative would be to have to override each token manually, which a) is still possible but mainly b) seems less favorable in terms of devX)
  • removes the @deprecated mark on ink and ghost tokens - while those are not expected in the theme, there is a use case for having true black and white tokens (currently at least for HCM)
  • adds HCM override for ink and ghost tokens (resets them to primitive black and white to ensure highest possible contrasts)
  • smaller cleanups

QA

  • review the testing example for EuiButton with fill={true} here - the custom overrides (code) should apply (and be inherited by the button tokens)(code)
Screen.Recording.2025-04-07.at.17.51.07.mov
  • (optional) verify manually locally that overrides work by adding overrides to the theme

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in both MacOS and Windows high contrast modes
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • 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
    • If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)

@mgadewoll mgadewoll self-assigned this Apr 7, 2025
@mgadewoll mgadewoll force-pushed the theme/279-support-theme-level-overrides branch 2 times, most recently from f0b75e1 to 988ed53 Compare April 8, 2025 09:12
@mgadewoll mgadewoll marked this pull request as ready for review April 8, 2025 13:31
@mgadewoll mgadewoll requested a review from a team as a code owner April 8, 2025 13:31
@mgadewoll mgadewoll requested a review from tkajtoch April 8, 2025 13:31
Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

The changes look and work good. I want to take a quick look on how the computed works in more detail and what performance implications it has before I approve this. This code is executed early and has direct impact on initial app render

Copy link
Member

@tkajtoch tkajtoch 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 computed performance and the difference is insignificant.

LGTM :shipit:

- ensures inherited colors are updated if the base is changed
- while ink and ghost are not defined for the theme, there is a use case for pure black and white tokens, expecially for HCM
- updates due to ink token change in HCM
- udpate after rebase
- ensures that provider level overrides apply ontop of theme level overrides
@mgadewoll mgadewoll force-pushed the theme/279-support-theme-level-overrides branch from 988ed53 to 95fe71d Compare April 15, 2025 15:58
@mgadewoll
Copy link
Contributor Author

mgadewoll commented Apr 15, 2025

@tkajtoch I rebased with main and additionally added another update (commit) that ensures the following override order (first item = lowest level; each following item overrides the previous):

  • theme level
  • provider level
  • theme HCM override (applies in HCM only)
  • provider level HCM override (applies in HCM only)

ℹ️ HCM specific overrides in high_contrast_overrides (file) are currently internal, fixed overrides that can't be manually overridden via the provider.

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

cc @mgadewoll

@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mgadewoll

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.

4 participants