search

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.36k stars 4.13k forks source link

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

Closed sarayourfriend closed 6 months ago

sarayourfriend commented 3 years ago

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 https://github.com/WordPress/gutenberg/issues/35744

Context connecting tasks

A list of high priority components to be refactored is tracked in https://github.com/WordPress/gutenberg/issues/35744

Documentation tasks

Deprecating `@emotion/css`

gziolo commented 3 years ago

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 (https://github.com/WordPress/gutenberg/issues/27484#issuecomment-739003356), 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 commented 3 years ago

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 commented 3 years ago

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:

https://github.com/WordPress/gutenberg/blob/b53c5e51fdb0db517f71eb80740cf698db344b5b/packages/components/src/ui/create-styles/create-style-system/constants.js#L4

jasmussen commented 3 years ago

I appreciate you clarifying that to me, thank you!

sarayourfriend commented 3 years ago

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

gziolo commented 3 years ago

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

ciampo commented 3 years ago

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

Opened https://github.com/WordPress/gutenberg/pull/31661 and https://github.com/WordPress/gutenberg/pull/31646

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

Opened draft PR https://github.com/WordPress/gutenberg/pull/31713

sarayourfriend commented 3 years ago

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: https://github.com/WordPress/gutenberg/pull/31238

sarayourfriend commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 6 months ago

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!