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

[8.0 Breaking Change] Emotion CSS-in-JS #88715

Closed
clintandrewhall opened this issue Jan 19, 2021 · 6 comments
Closed

[8.0 Breaking Change] Emotion CSS-in-JS #88715

clintandrewhall opened this issue Jan 19, 2021 · 6 comments
Labels
Breaking Change Feature:Canvas R&D Research and development ticket (not meant to produce code, but to make a decision) Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@clintandrewhall
Copy link
Contributor

clintandrewhall commented Jan 19, 2021

Summary of Exploration or Proposed Change

Canvas needs to remove public classnames from its elements in favor of strict contract. At present, users can inspect the DOM and write custom styles against the structure. While probably useful, it means that any changes to DOM structure or CSS classname is a breaking change between minor releases.

  1. EUI is pursuing Emotion CSS-in-JS. Canvas should follow suit to "privatize" its CSS.
  2. Canvas should be intentional in exposing CSS "hooks" to users. These should have API-level documentation and versioning.

There will be significant effort to bring this to our React components wholesale. We'll need to split this into phases:

  1. Renderers (highest priority, table stakes)
  2. Sidebar components
  3. Other UI components.

Questions

  • Curious if we can create a lint warning if any components are still using CSS classnames?
  • Should we combine this with CSS Modules somehow?

Findings

Summarize the findings of the research. Be as detailed as possible and appropriate. Link to any feature branches or other documents, as appropriate. Fill out the list below according to our planning strategy.

  • Proposed Priority: High
  • Level of Effort: High
  • Dependencies (if any):
    • EUI
    • Perhaps Platform?
  • Proposed development: [7.x]
@clintandrewhall clintandrewhall added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas R&D Research and development ticket (not meant to produce code, but to make a decision) Feature:Canvas Breaking Change 8.0.0 labels Jan 19, 2021
@clintandrewhall clintandrewhall self-assigned this Jan 19, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@clintandrewhall
Copy link
Contributor Author

@thompsongl has provided me with code to try the new API in Canvas. Stay tuned!

@clintandrewhall
Copy link
Contributor Author

clintandrewhall commented Feb 2, 2021

Notes

  • I have emotion working in Canvas, but it requires commenting out babel-plugin-styled-components in kbn-babel-preset/webpack_preset. Need to talk to @tsouza about how to properly disable it.
    • I need to add the jsx pragma regardless, which makes me think I have something configured incorrectly.
  • We're getting an error from emotion in the console: react_devtools_backend.js:2430 You are loading @emotion/react when it is already loaded. Running multiple instances may cause problems. This can happen if multiple versions are used, or if multiple builds of the same version are used.. @thompsongl is this due to emotion being init'd in EUI? Did you get this warning, too?
    • I synced the version numbers between Kibana and EUI, and it had no effect on the warning.
  • ReactDOM.render in our rendering code is once again biting us: we have to create the EuiThemeProvider in all of our render functions.
    • We'll need to create a wrapper component and use it in our renderers.
  • The EUI theme in the build @thompsongl provided is missing sizes, borders, etc. I'd be able to make a broader PoC if the theme was flushed out.

This has HUGE implications for Canvas, as we can use Emotion to create scoped class names... perhaps even with user-defined CSS. This will require more research.

@thompsongl
Copy link
Contributor

thompsongl commented Feb 2, 2021

@thompsongl is this due to emotion being init'd in EUI? Did you get this warning, too?

I do see this error now. Looking into options besides peerDependencies for @emotion/react

The EUI theme in the build @thompsongl provided is missing sizes, borders, etc. I'd be able to make a broader PoC if the theme was flushed out.

We're not set on the shape of the theme yet, but I can add some more categories with a temporary construction.

@thompsongl
Copy link
Contributor

@clintandrewhall Recent changes in thompsongl#3 contain resolutions to the You are loading @emotion/react when it is already loaded error, as well as pragma-less css usage with @emotion/react via @emotion/babel-preset-css-prop preset.

@mistic might be interested in the changes to kbn-babel-preset/webpack_preset.js. Modified the overrides approach that you mentioned; certainly open to feedback now or when this is closer to review.

@ThomThomson
Copy link
Contributor

Closing this for the time being as we aren't prioritizing lower impact tech debt for Canvas. We can reopen this if our priorities change in the future.

@ThomThomson ThomThomson closed this as not planned Won't fix, can't repro, duplicate, stale Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Feature:Canvas R&D Research and development ticket (not meant to produce code, but to make a decision) Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

No branches or pull requests

6 participants