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

[uiSettings/theme] restrict theme options based on KBN_OPTIMIZER_THEMES #94834

Merged
merged 8 commits into from
Mar 22, 2021

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Mar 17, 2021

Closes #94817

In development, only expose theme versions which have actually been built to avoid confusing state where v7 theme is selected but not built and things seem broken.

@spalger spalger added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 17, 2021
@spalger spalger force-pushed the implement/restrict-theme-options branch 3 times, most recently from 1ce0d2b to 7a6aa3f Compare March 17, 2021 20:21
@spalger spalger force-pushed the implement/restrict-theme-options branch from 7a6aa3f to 83a80c9 Compare March 17, 2021 20:21
@spalger
Copy link
Contributor Author

spalger commented Mar 17, 2021

@elasticmachine merge upstream

@spalger spalger marked this pull request as ready for review March 17, 2021 23:36
@spalger spalger requested a review from a team as a code owner March 17, 2021 23:36
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

Comment on lines +18 to +20
interface GetCoreSettingsOptions {
isDist?: boolean;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: why optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because this is a temporary thing that I plan to remove once we not longer need to switch back to the v7 theme, so I wanted to avoid updating all the tests which use the getCoreSettings() fn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And if you're mostly curious about the property specifically, I didn't really want to push people to define the property if they somehow end up calling getCoreSettings() as I'm not really sure where it's called but the places where it matters the right options are passed.

@spalger
Copy link
Contributor Author

spalger commented Mar 19, 2021

@elasticmachine merge upstream

@spalger
Copy link
Contributor Author

spalger commented Mar 22, 2021

@elasticmachine merge upstream

@spalger spalger enabled auto-merge (squash) March 22, 2021 17:28
@spalger spalger added the backport:skip This commit does not require backporting label Mar 22, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@spalger spalger merged commit f1708dd into elastic:master Mar 22, 2021
@spalger spalger deleted the implement/restrict-theme-options branch March 22, 2021 18:33
stephmilovic pushed a commit that referenced this pull request Mar 22, 2021
…ES (#94834)

Co-authored-by: spalger <spalger@users.noreply.github.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@cchaos
Copy link
Contributor

cchaos commented Apr 1, 2021

Thanks for this @spalger ! Do you think it would be possible to just disable the option instead of completely remove it? I think it's unclear why there's even a theme setting when there's only one option.

@spalger
Copy link
Contributor Author

spalger commented Apr 1, 2021

@cchaos I don't think there's any way to disable an option, and I don't really feel like adding that ability ATM, but how do you feel about #96069?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core/uiSettings/theme] inspect KBN_OPTIMIZER_THEMES in dev to determine options/defaults
5 participants