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

[EuiProvider] Detect OS/system light vs darkmode as a default #8026

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

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Sep 16, 2024

Summary

Nobody asked for this, but I thought I'd do it anyway! 🤠

EUI's theme/colorMode currently defaults to light mode. I've added a new provider that detects the current system light/dark mode setting and if no consumer colorMode prop has been set/overridden, will automatically default to the OS/system setting.

There's also a few src-docs cleanup in here (tagged as such in individual commits), and I'll shortly be doing another pass of our various docs that remove/deprecate all references to Sass setup.

QA

  • Go to https://eui.elastic.co/pr_8026/#/
  • Clear any saved local data for the site:
  • Go to System > Appearance > Set to dark mode
  • Refresh the page and confirm the docs site is in dark mode
  • Without refreshing the page, go to your system appearances again and set it to light mode
  • Tab back to the docs site and confirm that the site has switched over automatically
  • Go to the switcher and manually change the site back to light mode
  • Refresh the page and confirm that preferred setting is saved and EUI is always in light mode regardless of system setting

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in Chrome, Safari, Edge, and Firefox
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.

- doesn't have dynamic listening as a result, but oh well, that's how storybook's UI behaves so we might as well match it
- due to previous legacy theme font changes
… prop

- not really sure what either of these are doing, i'm guessing it's legacy code from before React context was more widely used
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

@cee-chen cee-chen marked this pull request as ready for review September 18, 2024 01:05
@cee-chen cee-chen requested a review from a team as a code owner September 18, 2024 01:05
@JasonStoltz
Copy link
Member

JasonStoltz commented Sep 20, 2024

I think this is a tremendous and much needed improvement.

This wouldn't be breaking change, correct? I think we currently require a colorMode prop of either light or dark -- and this lets consumers simply omit a color mode prop in order to take advantage of system-level settings.

If that is the case, I see no reason why we couldn't proceed with this PR. Directionally, it's where we're headed with Kibana and moving this logic to EUI lets us take advantage of this in all of our products.

@cee-chen
Copy link
Member Author

I think we currently require a colorMode prop of either light or dark -- and this lets consumers simply omit a color mode prop in order to take advantage of system-level settings.

No, we do not require the colorMode - it defaults to light mode if not passed.

This wouldn't be breaking change, correct?

Not a breaking change, correct. If consumers continue to not pass colorMode, then the default will be the system setting instead of the current default of light mode. If consumers pass a colorMode prop, that will always override / take precedence over our internal defaults.

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.

3 participants