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

[Emotion] Order EUI's CSS utilities after Sass styles #162365

Merged
merged 6 commits into from
Jul 25, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jul 20, 2023

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

  • Confirm Emotion output order is expected in head (EUI globals, All Emotion styles, Sass styles, then utilities last)

@cee-chen cee-chen marked this pull request as ready for review July 21, 2023 14:58
@cee-chen cee-chen requested review from a team as code owners July 21, 2023 14:58
@cee-chen cee-chen added release_note:skip Skip the PR/issue when compiling release notes EUI v8.10.0 labels Jul 21, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-team (EUI)

@cee-chen
Copy link
Contributor Author

@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!

@cee-chen
Copy link
Contributor Author

@elasticmachine merge upstream

@kc13greiner kc13greiner self-requested a review July 24, 2023 15:57
Copy link
Contributor

@vadimkibana vadimkibana left a comment

Choose a reason for hiding this comment

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

Code changes LGTM.

@cee-chen
Copy link
Contributor Author

Thanks y'all for the reviews so far! @kc13greiner - any chance you'll have time sometime today to review? 🙏

@cee-chen
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
interactiveSetup 50 53 +3
kibanaReact 310 313 +3
kibanaUtils 168 171 +3
total +9

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/core-base-common 3 4 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 361.5KB 361.7KB +149.0B
interactiveSetup 58.6KB 58.9KB +225.0B
kibanaReact 51.3KB 51.5KB +220.0B
kibanaUtils 70.8KB 71.1KB +235.0B
total +829.0B
Unknown metric groups

API count

id before after diff
@kbn/core-base-common 12 13 +1

History

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

Copy link
Contributor

@kc13greiner kc13greiner left a comment

Choose a reason for hiding this comment

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

LGTM!

@cee-chen cee-chen merged commit ca9b4d0 into elastic:main Jul 25, 2023
@cee-chen cee-chen deleted the eui-utils-cache branch July 25, 2023 17:37
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 25, 2023
@tonyghiani
Copy link
Contributor

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!

clintandrewhall added a commit that referenced this pull request Jul 31, 2023
## 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]>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
## 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]>
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
## 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]>
cee-chen added a commit that referenced this pull request Aug 4, 2023
…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]>
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 EUI release_note:skip Skip the PR/issue when compiling release notes v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants