-
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
Add basic support for experimental theme switching #199748
Conversation
dd3cb6c
to
6e85833
Compare
…der to only load themes that are supposed to be bundled
6b645f9
to
e385ad2
Compare
…tstrap/get_theme_tag.test.ts Co-authored-by: Rudolf Meijering <[email protected]>
…tstrap/get_theme_tag.test.ts Co-authored-by: Rudolf Meijering <[email protected]>
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.
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.
LGTM for the Threat Hunting Investigations team
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.
Good job on this one @tkajtoch . Bundles do now LGTM and the changes were tested on top of the webpack v5 upgrade PR
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.
reviewed x-pack/plugins/observability_solution/metrics_data_access/public/test_utils/use_global_storybook_theme.tsx on behalf of @elastic/obs-knowledge-team. LGTM.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
cc @tkajtoch |
## Summary This PR adds basic support for theme switching to test experimental themes internally. It defines a new `theme:name` setting which is read-only (and thus hidden) by default, and needs `uiSettings.experimental.themeSwitcherEnabled: true` config setting to become visible. The implementation and the way the theme switcher feature is enabled will likely change in the upcoming weeks when we iron out the details, but the way it works right now is sufficient for testing and that's what the immediate goal is. Please note this PR includes mostly setup work, and no additional themes have been added here. To make reviewing slightly easier, these will be added separately. The feature and settings should be undocumented for the time being and disabled by default. --- As you might notice, there's a new setting added despite already having `theme:version`. We decided to introduce a new setting instead of resurrecting an existing one for better naming and flexibility and to reduce the risk of breaking things that might still use the old setting value (hardcoded to `v8` at the moment). The current plan is to remove `theme:version` in 9.0. The theme_loader got refactored to only bundle active themes coming from `KBN_OPTIMIZER_THEMES` (defaulting to `v8light` and `v8dark`) instead of `ALL_THEMES` that would significantly increase bundle sizes even when the experimental themes were not enabled. Considering there's a SASS to CSS-in-JS migration happening right now, we don't need to worry about Kibana bundles growing in size when a new theme is officially added. ### Checklist Delete any items that are not applicable to this PR. - [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 ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Clint Andrew Hall <[email protected]> Co-authored-by: Rudolf Meijering <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Tiago Costa <[email protected]>
## Summary This PR adds basic support for theme switching to test experimental themes internally. It defines a new `theme:name` setting which is read-only (and thus hidden) by default, and needs `uiSettings.experimental.themeSwitcherEnabled: true` config setting to become visible. The implementation and the way the theme switcher feature is enabled will likely change in the upcoming weeks when we iron out the details, but the way it works right now is sufficient for testing and that's what the immediate goal is. Please note this PR includes mostly setup work, and no additional themes have been added here. To make reviewing slightly easier, these will be added separately. The feature and settings should be undocumented for the time being and disabled by default. --- As you might notice, there's a new setting added despite already having `theme:version`. We decided to introduce a new setting instead of resurrecting an existing one for better naming and flexibility and to reduce the risk of breaking things that might still use the old setting value (hardcoded to `v8` at the moment). The current plan is to remove `theme:version` in 9.0. The theme_loader got refactored to only bundle active themes coming from `KBN_OPTIMIZER_THEMES` (defaulting to `v8light` and `v8dark`) instead of `ALL_THEMES` that would significantly increase bundle sizes even when the experimental themes were not enabled. Considering there's a SASS to CSS-in-JS migration happening right now, we don't need to worry about Kibana bundles growing in size when a new theme is officially added. ### Checklist Delete any items that are not applicable to this PR. - [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 ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Clint Andrew Hall <[email protected]> Co-authored-by: Rudolf Meijering <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Tiago Costa <[email protected]>
Summary
This PR adds basic support for theme switching to test experimental themes internally. It defines a new
theme:name
setting which is read-only (and thus hidden) by default, and needsuiSettings.experimental.themeSwitcherEnabled: true
config setting to become visible. The implementation and the way the theme switcher feature is enabled will likely change in the upcoming weeks when we iron out the details, but the way it works right now is sufficient for testing and that's what the immediate goal is.Please note this PR includes mostly setup work, and no additional themes have been added here. To make reviewing slightly easier, these will be added separately.
The feature and settings should be undocumented for the time being and disabled by default.
As you might notice, there's a new setting added despite already having
theme:version
. We decided to introduce a new setting instead of resurrecting an existing one for better naming and flexibility and to reduce the risk of breaking things that might still use the old setting value (hardcoded tov8
at the moment). The current plan is to removetheme:version
in 9.0.The theme_loader got refactored to only bundle active themes coming from
KBN_OPTIMIZER_THEMES
(defaulting tov8light
andv8dark
) instead ofALL_THEMES
that would significantly increase bundle sizes even when the experimental themes were not enabled. Considering there's a SASS to CSS-in-JS migration happening right now, we don't need to worry about Kibana bundles growing in size when a new theme is officially added.Checklist
Delete any items that are not applicable to this PR.
For maintainers