-
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
[Emotion] Order EUI's CSS utilities after Sass styles #162365
Conversation
Pinging @elastic/eui-team (EUI) |
@tonyghiani - I'm having difficulty reproducing the exact screen/bug you reported. Do you mind quickly checking that this PR does indeed fix your issue? Thanks a million! |
@elasticmachine merge upstream |
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.
Code changes LGTM.
Thanks y'all for the reviews so far! @kc13greiner - any chance you'll have time sometime today to review? 🙏 |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Page load bundle
History
To update your PR or re-run it, just comment with: |
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!
Hey @cee-chen sorry for the late reply, I just tried to reproduce the bug and it is correctly solved by your fix, thanks for this work! |
## Summary As titled. This PR corrects a pair of mistakes I made before committing #161914 that @dgieselaar identified shortly thereafter. - I had tested Storybook extensively, but after I rebased, I changed the render context, and I forgot to update the `decorator` in Storybook. This meant Emotion styles worked, but the EUI styles were missing. - In addition, when I rebased, I missed the addition of the utils cache that had been added by EUI. - Interestingly, #162365 missed adding the cache `meta` tag to the template. Emotion simply added the styles to the `head`, but it's best to reproduce what we see in Kibana. So I've corrected that, as well. - While creating the PR, I went ahead and addressed [feedback](#161914 (comment)) from @cee-chen on the original PR./ Sorry if anyone was confused by the sudden drop in styles in their Storybooks. Should be resolved now. Thanks! --------- Co-authored-by: kibanamachine <[email protected]>
## Summary Follow up to elastic#161592 Some remaining EUI components that are still in Sass unfortunately need to respect EUI's global CSS utilities (e.g. `.eui-yScroll`, `.eui-textTruncate` - [full list here](https://elastic.github.io/eui/#/utilities/css-utility-classes)). Creating a separate utilities cache and insertion point should solve the CSS order/specificity issues. ### Checklist - [x] Confirm Emotion output order is expected in head (EUI globals, All Emotion styles, Sass styles, then utilities last) --------- Co-authored-by: kibanamachine <[email protected]>
## Summary As titled. This PR corrects a pair of mistakes I made before committing elastic#161914 that @dgieselaar identified shortly thereafter. - I had tested Storybook extensively, but after I rebased, I changed the render context, and I forgot to update the `decorator` in Storybook. This meant Emotion styles worked, but the EUI styles were missing. - In addition, when I rebased, I missed the addition of the utils cache that had been added by EUI. - Interestingly, elastic#162365 missed adding the cache `meta` tag to the template. Emotion simply added the styles to the `head`, but it's best to reproduce what we see in Kibana. So I've corrected that, as well. - While creating the PR, I went ahead and addressed [feedback](elastic#161914 (comment)) from @cee-chen on the original PR./ Sorry if anyone was confused by the sudden drop in styles in their Storybooks. Should be resolved now. Thanks! --------- Co-authored-by: kibanamachine <[email protected]>
…banaRenderContextProvider (#163103) ## Summary Unfortunately, #161914 regressed #162365 in that many plugins and their Emotion styles (including EUI emotion styles) are now missing a cache and are being appended to to the end of the document `<head>` as opposed to within `<meta name="emotion">`. What appears to be happening is many plugins are using a parent `<KibanaThemeProvider>` but **not** a parent `<KibanaRootContextProvider>` (not sure if this work is TBD or in progress). This means that a parent `<EuiProvider>`, (which determines the cache insertion of child Emotion styled components) is missing, which is causing several CSS specificity bugs, e.g. around datagrid. As a somewhat-bandaid-y fix, I've bogarted EUI's nested provider context to check if the theme provider has a parent `EuiProvider`, and if it doesn't, to use `KibanaEuiProvider` instead of `KibanaThemeProvider`. This should set up the caches and context if needed, or otherwise simply use the original `KibanaThemeProvider` component. --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Clint Andrew Hall <[email protected]>
Summary
Follow up to #161592
Some remaining EUI components that are still in Sass unfortunately need to respect EUI's global CSS utilities (e.g.
.eui-yScroll
,.eui-textTruncate
- full list here). Creating a separate utilities cache and insertion point should solve the CSS order/specificity issues.Checklist