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

Fully integrating vanilla Emotion as the canonical style system for CSS-in-JS in Gutenberg #30503

Closed
sarayourfriend opened this issue Apr 5, 2021 · 12 comments
Assignees
Labels
[Feature] Component System WordPress component system [Status] In Progress Tracking issues with work in progress [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.

Comments

@sarayourfriend
Copy link
Contributor

sarayourfriend commented Apr 5, 2021

Note: This issue has been updated significantly, please view the revision history for insight into what's changed!

This is the next step in fully ingrating Emotion as the canonical style system for Gutenberg.

Recently we made the decision to ditch the new custom style system for the following reasons:

  1. It presented too many difficulties in upgrading Emotion to version 11
  2. It implemented a custom iframe solution that diverged from Emotion's upstream solution
  3. It potentially introduced too many new ways of doing things like themeing and CSS custom property management that would produce too many new public APIs
  4. It would introduce a non-trivial maintenance burden on the Gutenberg core contributors

Because @youknowriad was able to figure out how to integrate the StyleFrame component using the methods from upstream Emotion for iframe support (thanks Riad!) we're able to fully commit to vanilla Emotion for the Gutenberg style system. This means we do not need a custom wrapper around it.

What are we losing by doing this?

Basically "base styles" which were styles that were being applied to each and every element produced by the style system. Practically this meant the ability to force all components to respect reduced motion automagically. I think we can figure out a solution around this using injectGlobal and adding it to the StyleProvider, but it's not a blocker, just something to call out.

Okay, so what are the next steps?

Miscellaneous tasks

Component uplifting tasks

CSS-in-JS tasks

A list of high priority components to be refactored is tracked in #35744

  • Convert components to CSS-in-JS
    • Add each component as it is converted to this list here
    • Button
    • ButtonGroup
    • Draggable
    • CheckboxControl
    • etc...

Context connecting tasks

A list of high priority components to be refactored is tracked in #35744

  • Connect each component to the ui/context
    • Add each component as it is connected to this list
    • Button
    • ButtonGroup
    • etc..

Documentation tasks

  • Update root @wordpress/components README.md to describe the structure of the package including the differences between primitive components (like Text) and composed components (like ColorPicker)
    • Create a tracking issue for documenting this Document @wordpress/components structure #33111
    • Document each component's type
    • Update each component's README.md with the new information
    • Update the root README.md with the general description of the component types and their implications

Deprecating `@emotion/css`

@gziolo
Copy link
Member

gziolo commented Apr 6, 2021

Thank you @sarayourfriend for actively seeking better ways to integrate the new Component System. I think it makes perfect sense to try a bit different approach based on all the experience you collected when working on integration so far. In fact, it's closer to what I imagined in the early phase of planning. I hoped we can switch a few lower-level components to their revised implementation and bring all the necessary machinery to make them work with all the new features the that Component System brings. At that stage (#27484 (comment)), I wasn't aware of the limitation you listed but it only shows that the sooner you start integrating components the more you learn about the potential issues that should be addressed with higher priority.

I agree that #30509 is the most important blocker to resolve before we can start using some of the components that are already added to the @wordpress/components/ui folder 👍🏻

@jasmussen
Copy link
Contributor

Question, as the new system is getting integrated, does it make sense to rename --wp-g2- prefix to simply --wp-, to match existing values?

@sarayourfriend
Copy link
Contributor Author

Question, as the new system is getting integrated, does it make sense to rename --wp-g2- prefix to simply --wp-, to match existing values?

@jasmussen Yes I think ultimately that should be the goal. For now the new style system, as it is integrated, has the --wp-experimental prefix:

@mtias mtias added the [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues. label Apr 12, 2021
@jasmussen
Copy link
Contributor

I appreciate you clarifying that to me, thank you!

@sarayourfriend sarayourfriend changed the title Integrate the new style system Fully ingrating vanilla Emotion as the canonical style system for CSS-in-JS in Gutenberg Apr 23, 2021
@sarayourfriend
Copy link
Contributor Author

Note! I've updated the issue to reflect the current plan. Please review when y'all have time @mtias @gziolo @youknowriad. Thanks!

@gziolo
Copy link
Member

gziolo commented Apr 23, 2021

Thank you for the update. It looks like everything is on track. 👍🏻

@ciampo
Copy link
Contributor

ciampo commented May 11, 2021

Remove color and config utility functions and just use the COLORS and CONFIG objects directly

Opened #31661 and #31646

Deprecate the various isPrimary,isSecondary etc. props on Button in favor of a single variant prop.

Opened draft PR #31713

@sarayourfriend
Copy link
Contributor Author

sarayourfriend commented May 20, 2021

Migrating a G2 component into Gutenberg.

Helpful example PRs:

The rough outline of the process is as follows

  1. Copy the component from the g2 repo into packages/components/src/component-name (use kebab case for the folder)
  2. Add the folder to components/tsconfig.json#includes
  3. Rename the hook file to hook.ts and the component file to component.ts(x) and the styles file to styles.ts.
  4. Update the imports to import cx and css from emotion.
  5. Remove react imports and change them to be from @wordpress/element
  6. Remove the ui.get usage and replace it with a combo of the COLORS and CONFIG objects (you can see an example of CONFIG in the Elevation PR I linked. COLORS is very similar)
  7. Any other functions called off of ui that don’t currently have a utility in the Gutenberg repo need to have a utility created.
  8. Move any functions imported from @wp-g2/utils directly into the Gutenberg repository under the ui/utils folder (like just copy them from G2 into GB)
  9. Change @wp-g2/context imports to import from ui/context
  10. Fix/add types if they’re missing (they almost certainly are missing, but you can find mostly complete Prop types by searching ComponentName.d.ts in the G2 repo). Also be sure to add the children type which is implicit in the G2 repository but needs to be explicit in the Gutenberg repository.

Usages of the ui.$ function can be completely removed. Usages of createToken need to be replaced with functions that return CSS based on the variables, similar to the Surface PR: #31238

@sarayourfriend
Copy link
Contributor Author

Regarding base styles, I've added a new comment copied here for ease:

Should we actually do this? View isn't the basis of the styled components so this will only cover a small set of components that would actually get the base styles. Should we just use injectGlobal to apply the base styles to * with an !important on the reduced motion ones?

What do y'all think?

cc @ciampo @youknowriad

The goal is that we want reduced motion to be respected automagically, but maybe there's no way to do that and we just need to use the useReducedMotion hook everywhere 🤔 But I don't know how well that will work in CSS-in-JS as we don't want to depend on runtime changes 🤔 We should be able to apply the media query and potentially override animations if the environment variable is present to disable animations.

Overall just a little stuck thinking about this and curious what other people think.

@ciampo
Copy link
Contributor

ciampo commented May 24, 2021

I may lack some perspective of the high-level strategy for the components package, but it makes sense to me that, whatever solution we may go for, we would be able to affect all components. Having implementation discrepancies in between components would make it quite hard to maintain and implement features effectively.

View isn't the basis of the styled components so this will only cover a small set of components that would actually get the base styles

Hopefully not a silly question, but — Can't we make changes so that all components are based on View? Would it be too complicated/disruptive?

[...] But I don't know how well that will work in CSS-in-JS as we don't want to depend on runtime changes 🤔 We should be able to apply the media query and potentially override animations if the environment variable is present to disable animations.

Wouldn't be able to wrap some CSS into a @media (prefers-reduced-motion) { ... } query without relying on the useReducedMotion hook's dynamic changes?

@sarayourfriend
Copy link
Contributor Author

sarayourfriend commented May 24, 2021

Hopefully not a silly question, but — Can't we make changes so that all components are based on View? Would it be too complicated/disruptive?

Not a silly question. All of the new components ultimately render a View of some sort, but along with that View they may render a styled component. I guess we could make the styled components all inherit from View (styled( View )`` for example) but that's not semantic Emotion in any way at all and it would make getting semantic elements out of styled more difficult (you'd have to pass the as prop rather than just calling off of styled).

Previously, when we were using our own wrapper around Emotion, we were able to make all styled components ultimately inherit from View, but that's not possible without the wrapper around Emotion generating our own custom styled object as far as I know.

Wouldn't be able to wrap some CSS into a @media (prefers-reduced-motion) { ... } query without relying on the useReducedMotion hook's dynamic changes?

As far as I understand it, the FORCE_REDUCED_MOTION environment variable doesn't affect the prefers-reduced-motion query at all, so we always have to check the FORCED_REDUCED_MOTION variable in order for it to work with e2e test expectations. Maybe that's an incorrect understanding though, @gziolo @youknowriad can you confirm this one way or the other? Does the env variable effect the media query?

@annezazu
Copy link
Contributor

Closing this out as it appears much of this work has been completed or is no longer relevant today (it's been three years since last updated). Happy to reopen if that's incorrect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Status] In Progress Tracking issues with work in progress [Type] Tracking Issue Tactical breakdown of efforts across the codebase and/or tied to Overview issues.
Projects
Status: Done 🎉
Development

No branches or pull requests

6 participants