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] Change location of fallback Emotion cache #6202

Merged
merged 5 commits into from
Sep 6, 2022

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Sep 1, 2022

Summary

Having the fallback cache configured for each EuiCacheProvider broke the context "bubbling" fallback assumed by EuiProvider, resulting in the defaultCache in EuiProvider not being the catch-all location for Emotion styles. The net effect is that global styles were being inserted after component styles when not configured with EuiProvider.cache.global (such is the case in the docs).

The fix is to move the fallback to be the default prop value in EuiProvider rather than EuiCacheProvider.
Also, the defaultCache will always get compat=true, even if consumers forget to add it.

Before
Screen Shot 2022-09-01 at 3 11 17 PM

After
Screen Shot 2022-09-01 at 3 11 47 PM

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • A changelog entry exists and is marked appropriately

@thompsongl thompsongl mentioned this pull request Sep 1, 2022
5 tasks
@thompsongl thompsongl requested a review from cee-chen September 1, 2022 20:37
defaultCache = cache.default;
globalCache = cache.global;
utilityCache = cache.utility;
}
}
return (
<EuiCacheProvider cache={defaultCache}>
<EuiCacheProvider cache={defaultCache ?? fallbackCache}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? it should be covered by the = fallbackCache above, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not if some one configures like cache={{ global: globalCache }}
This is for the granular case.

Comment on lines +87 to 90
if (cache.default) {
cache.default.compat = true;
}
defaultCache = cache.default;
Copy link
Contributor

Choose a reason for hiding this comment

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

oh actually i think i see (re above comment)... we make each nested cache optional.

tbh though, why not make cache.default required and skip the conditional check? or make them all required? how likely are consumers to pass a cache just for global styles or just for utility styles?

Copy link
Contributor

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Thanks for figuring this out! I left some thinking-out-loud comments, but definitely nothing blocking

@kibanamachine
Copy link

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

@thompsongl thompsongl marked this pull request as ready for review September 6, 2022 15:16
@thompsongl thompsongl enabled auto-merge (squash) September 6, 2022 15:16
@kibanamachine
Copy link

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

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