Skip to content

[CSS-in-JS] Granular insertion; Utility component for classes #5853

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
Merged
merged 40 commits into from
Jun 22, 2022
Merged

[CSS-in-JS] Granular insertion; Utility component for classes #5853

merged 40 commits into from
Jun 22, 2022

Conversation

@thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Apr 28, 2022 •
edited
Loading

Summary

Add granular EuiProvider configuration for where to insert which styles

The cache prop can now accept several properties to fine tune control over where Emotion styles get inserted.

  • default will encompass all Emotion styles, including consumer defined appliction styles, not handled by nested cache instances.
  • global will scope all EUI global and reset styles.
  • utility will scope all EUI utility class styles.

A cache instance provided as the sole value will function the same as the default cache. This prevents a breaking change and will be useful for when EUI no longer ships compiled CSS.

Docs site
Updates the webpack config to inject the compiled CSS to a certain location: after EUI Emotion styles but before utility classes in the docs.

Takes advantage of the new cache props to order style injection.

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
  • Added documentation
  • 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
  • Revert example utility class

@kibanamachine
Copy link

kibanamachine commented Apr 28, 2022

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

@kibanamachine
Copy link

kibanamachine commented Apr 29, 2022

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

cee-chen
cee-chen reviewed Apr 29, 2022
View reviewed changes
@chandlerprall
Copy link
Contributor

chandlerprall commented Apr 29, 2022

Can you put together a wiki page (or update an existing one) with how the cache setup would work, both from EUI's perspective (for us+contributors) and from an app's perspective if they want/need to use these specific caches. I think seeing the documentation alongside the code changes will help make sense of the direction.

cee-chen
cee-chen reviewed Apr 29, 2022
View reviewed changes
cee-chen
cee-chen reviewed Apr 29, 2022
View reviewed changes
cee-chen
cee-chen reviewed Apr 29, 2022
View reviewed changes
@thompsongl
Copy link
Contributor Author

thompsongl commented May 2, 2022

I haven't considered naming all that much yet. The current state was really just to gauge reaction to using a provider in every component and discuss HOC vs. hook usage.

@chandlerprall
Copy link
Contributor

chandlerprall commented May 3, 2022

@constancecchen provided some additional thoughts today on the HOC approach: it would affect all of the snapshots and require shallow rendered components to dive into the wrapped component.

@cee-chen
Copy link
Contributor

cee-chen commented May 3, 2022

🚢 After today's sync I am on board with the necessity for component-cache HOC wrappers - most of my concerns about snapshots and mounted behavior should ideally transition away in any case with a gradual move away from Enzyme/mount and towards React Testing Library. (RIP shallow, my best friend)

@kibanamachine
Copy link

kibanamachine commented May 6, 2022

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

@cee-chen cee-chen mentioned this pull request May 6, 2022
Merged
4 tasks
@kibanamachine
Copy link

kibanamachine commented May 11, 2022

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

@thompsongl
Copy link
Contributor Author

thompsongl commented Jun 6, 2022

Not sure if there's an easier way to do this, but could we publish this specific branch to NPM with a beta tag and then open a draft Kibana CI PR pointing at that beta release?

Working on a Kibana compatibility PR right now, which is kind of funky because it currently needs to be based on elastic/kibana#132257 branch so that we don't get false failures.
Once that merges I'll set up a test PR in Kibana. In the meantime I'll figure out the right way to make a pre-release release on npm.

@kibanamachine
Copy link

kibanamachine commented Jun 7, 2022

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

@kibanamachine
Copy link

kibanamachine commented Jun 13, 2022

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

@kibanamachine
Copy link

kibanamachine commented Jun 14, 2022

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

@thompsongl thompsongl mentioned this pull request Jun 15, 2022
Merged
@kibanamachine
Copy link

kibanamachine commented Jun 20, 2022

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

cee-chen
cee-chen reviewed Jun 21, 2022
View reviewed changes
cee-chen
cee-chen reviewed Jun 21, 2022
View reviewed changes
cee-chen
cee-chen reviewed Jun 21, 2022
View reviewed changes
cee-chen
cee-chen reviewed Jun 21, 2022
View reviewed changes
cee-chen
cee-chen reviewed Jun 21, 2022
View reviewed changes
src/components/provider/cache/cache_provider.tsx
cache,
children,
}: PropsWithChildren<EuiCacheProviderProps>) => {
return children && cache ? (
Copy link
Contributor

@cee-chen cee-chen Jun 21, 2022

Choose a reason for hiding this comment

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

Just wondering, is it necessary to check for children here or just cache? The ternary fallthrough would just render an empty fragment in any case so maybe I'm overthinking this

Copy link
Contributor

@cee-chen cee-chen Jun 21, 2022

Choose a reason for hiding this comment

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

Another question while I'm here: should we have a basic set of unit tests for the EuiCacheProvider component?

cee-chen
cee-chen approved these changes Jun 21, 2022
View reviewed changes
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.

LGTM! I think my only remaining comments would be maybe unit tests for the new EuiCacheProvider component, but that's optional / I'll leave it up to you.

@kibanamachine
Copy link

kibanamachine commented Jun 21, 2022

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

@kibanamachine
Copy link

kibanamachine commented Jun 21, 2022

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

@thompsongl thompsongl merged commit a91b47d into elastic:main Jun 22, 2022
@cee-chen cee-chen mentioned this pull request Jun 22, 2022
Merged
@thompsongl thompsongl mentioned this pull request Jun 22, 2022
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@cee-chen cee-chen cee-chen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

emotion

Projects

None yet

Milestone

Elastic Stack 8.4

Development

Successfully merging this pull request may close these issues.

4 participants

@thompsongl @kibanamachine @chandlerprall @cee-chen