-
Notifications
You must be signed in to change notification settings - Fork 843
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
Conversation
defaultCache = cache.default; | ||
globalCache = cache.global; | ||
utilityCache = cache.utility; | ||
} | ||
} | ||
return ( | ||
<EuiCacheProvider cache={defaultCache}> | ||
<EuiCacheProvider cache={defaultCache ?? fallbackCache}> |
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.
Is this necessary? it should be covered by the = fallbackCache
above, right?
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.
Not if some one configures like cache={{ global: globalCache }}
This is for the granular case.
if (cache.default) { | ||
cache.default.compat = true; | ||
} | ||
defaultCache = cache.default; |
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.
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?
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.
Thanks for figuring this out! I left some thinking-out-loud comments, but definitely nothing blocking
Preview documentation changes for this PR: https://eui.elastic.co/pr_6202/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_6202/ |
Summary
Having the fallback cache configured for each
EuiCacheProvider
broke the context "bubbling" fallback assumed byEuiProvider
, resulting in thedefaultCache
inEuiProvider
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 withEuiProvider.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 thanEuiCacheProvider
.Also, the
defaultCache
will always getcompat=true
, even if consumers forget to add it.Before
![Screen Shot 2022-09-01 at 3 11 17 PM](https://user-images.githubusercontent.com/2728212/188003349-7bb31293-dd28-4ae2-8574-0bc228bcf748.png)
After
![Screen Shot 2022-09-01 at 3 11 47 PM](https://user-images.githubusercontent.com/2728212/188003434-0f7e399b-70e4-4ce3-bbc6-f4496576910e.png)
Checklist