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.

Sign up for GitHub

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

Jump to bottom

[EuiProvider] Add globalStyles prop for customization #5497

Merged
merged 16 commits into from
Jan 19, 2022
Merged

[EuiProvider] Add globalStyles prop for customization #5497

merged 16 commits into from
Jan 19, 2022

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Dec 21, 2021 •
edited
Loading

Summary

  • Exports EuiGlobalStyles to allow for custom composition
  • Adds globalStyles prop to EuiProvider to enable custom or no global styles
    • For future cases where a consumer is using a totally custom theme, they will have control over global styles
    • For Kibana, where multiple EuiProvider instances are loaded and we want to eliminate duplicating global styles
  • Updates @emotion dependencies

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@thompsongl thompsongl changed the title Provider globalstyles [EuiProvider] Add globalStyles prop for customization Dec 21, 2021
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5497/

@thompsongl thompsongl marked this pull request as ready for review December 22, 2021 17:52
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5497/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5497/

cchaos
cchaos reviewed Jan 6, 2022
View reviewed changes
src/components/provider/provider.tsx
cache?: EmotionCache;
}

export const EuiProvider = <T extends {} = {}>({
cache,
theme = EuiThemeAmsterdam,
globalStyles: GlobalStyles = EuiGlobalStyles,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the default be undefined instead? My guess is that most consumer's of multiple providers won't realize they need to remove the global styles. But likely, they'll notice they need to add it in order to fix the base styles.

Or should we be recommending two different providers. For instance this is just the theme provider, so should Kibana's multiple instances just switch to using the theme provider?

Copy link
Contributor Author

@thompsongl thompsongl Jan 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The summary might be overly concerned with multiple providers. The reasons for having default global styles but allowing for their removal are that:

  • Custom themes need to remove default global styles
  • With the exception of Kibana, apps will only have one EuiProvider instance
  • In the future, EuiProvider will likely also include i18n and other features
    • The Kibana provider that wraps EuiProvider will need access to all that data
    • That is, EuiThemeProvider isn't enough, but we need a way to prevent duplicate global styles (again, this very likely only affect Kibana)

Copy link
Contributor

Choose a reason for hiding this comment

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

very likely only affect Kibana

This is specifically where I worry that Kibana engineers will not know that they need to remove the global styles, making them compound or override. What is your plan to educate those engineers about this workaround?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All instances of EuiProvider (4 right now; unlikely to increase) are in mount-point components maintained by the core team. They all use the same pattern and I can leave comments in the 3 that do not need global styles explaining the difference.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5497/

cchaos
cchaos reviewed Jan 12, 2022
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Ok, I tried to re-word that example text to better explain, based on your explanation.

src-docs/src/views/provider/provider.md Outdated

A reset stylesheet and the global EUI styles are applied via Emotion. To prevent loading these styles from loading, pass `theme={null}` to the provider.
A reset stylesheet and the global EUI styles are applied via Emotion. To prevent loading these styles from loading, pass `globalStyles={false}` to the provider. If you are using the legacy theme, we recommend passing `theme={null}` to achieve similar results.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
A reset stylesheet and the global EUI styles are applied via Emotion. To prevent loading these styles from loading, pass `globalStyles={false}` to the provider. If you are using the legacy theme, we recommend passing `theme={null}` to achieve similar results.
The provider includes general reset and global styles, applied via Emotion. These only need to be applied **once** so to prevent these styles from loading in nested instances of the provider, pass `globalStyles={false}`.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also really need to move this doc to real docs example page so that we can change the provider setup example based on the currently selected theme. I don't think anyone will catch that sentence you added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Converted the docs and made the code blocks update based on the docs theme selections.

thompsongl and others added 2 commits January 12, 2022 18:04
@thompsongl @cchaos
Update src-docs/src/views/provider/provider.md
3cd86d0 
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@thompsongl
Merge branch 'main' into provider-globalstyles
8b4d7f8
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5497/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5497/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5497/

@thompsongl thompsongl added this to the Elastic Stack 8.1 milestone Jan 13, 2022
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5497/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5497/

chandlerprall
chandlerprall approved these changes Jan 19, 2022
View reviewed changes
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Pulled & tested locally

@thompsongl thompsongl merged commit 55194c2 into elastic:main Jan 19, 2022
@thompsongl thompsongl mentioned this pull request Jan 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

@cchaos cchaos Awaiting requested review from cchaos

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Elastic Stack 8.1
Development

Successfully merging this pull request may close these issues.
4 participants
@thompsongl @kibanamachine @chandlerprall @cchaos