-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ML] Replace useIsDarkTheme()
with EUI's colorMode
.
#205079
Conversation
42fab07
to
b721a55
Compare
3bd352e
to
3e1f0a9
Compare
Pinging @elastic/ml-ui (:ml) |
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.
Tested, LGTM 👍
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.
Code changes LGTM
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
History
cc @walterra |
## Summary This PR replaces the `useIsDarkTheme()` hook with EUI's `colorMode`. Since this was the last hook in `@kbn/ml-kibana-theme` left this removes the whole package too. Note that the hook subscribed to an observable and was able to update the theme in place. EUI's `colorMode` will only be updated after a page refresh. Since updating the Kibana advanced setting to enable dark mode requires a full page refresh too, I guess it's fair to remove this behavior in favor of this simplification. In the long run we should aim for getting rid of these checks altogether and rely on dark-mode-aware EUI tokens. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations.
## Summary This PR replaces the `useIsDarkTheme()` hook with EUI's `colorMode`. Since this was the last hook in `@kbn/ml-kibana-theme` left this removes the whole package too. Note that the hook subscribed to an observable and was able to update the theme in place. EUI's `colorMode` will only be updated after a page refresh. Since updating the Kibana advanced setting to enable dark mode requires a full page refresh too, I guess it's fair to remove this behavior in favor of this simplification. In the long run we should aim for getting rid of these checks altogether and rely on dark-mode-aware EUI tokens. ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations.
Summary
This PR replaces the
useIsDarkTheme()
hook with EUI'scolorMode
. Since this was the last hook in@kbn/ml-kibana-theme
left this removes the whole package too.Note that the hook subscribed to an observable and was able to update the theme in place. EUI's
colorMode
will only be updated after a page refresh. Since updating the Kibana advanced setting to enable dark mode requires a full page refresh too, I guess it's fair to remove this behavior in favor of this simplification.In the long run we should aim for getting rid of these checks altogether and rely on dark-mode-aware EUI tokens.
Checklist
release_note:breaking
label should be applied in these situations.