Animate: Use CSS-in-JS #25754
Animate: Use CSS-in-JS #25754
Conversation
sarayourfriend
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
|
/cc @ItsJonQ |
| [ 'is-from-' + yAxis ]: yAxis !== 'middle', | ||
| } ), | ||
| } ); | ||
| if ( yAxis === 'middle' ) { |
There was a problem hiding this comment.
I believe this value was used for consistency with the Popover position props but I may be wrong.
There was a problem hiding this comment.
Hmm, should it be kept and still filtered out then? It does appear to line up with the Popover position props.
youknowriad
added
[Feature] UI Components
|
I've seen that you opened two PRs about CSS in JS refactors, I have nothing against these but I'm curious about the motivation. |
|
@youknowriad Maybe I should open an issue to open discussion about this, but my motivation here is to make it possible to progressively load the CSS for each of these components. Right now the CSS for the entire package is bundled as a single file during the build, rather than allowing each component to progressively load the styles as needed. By moving the styles into CSS-in-JS it will make it possible for the styles for each component to be reduced (which is a benefit even if for some reason we cannot eventually fully remove the built stylesheet). Does this need more discussion before I can continue this work, shall I open an issue? My understanding is that this was already the direction we were heading in with newer components, so I figured I would take advantage of that and uplift as many of the existing ones as I could. |
|
@saramarcondes Haiii!! Thank for you for helping out with this! ❤️ ⏭ ⏪ RTL HandlingAs a whole, RTL in CSS-in-JS is tricky, and isn't standardized (compared to other CSS-in-JS approaches). The current CSS-in-JS implementation within Gutenberg is incomplete - at least compared to Sass which includes RTL conversion via PostCSS during build. With runtime compiled styles (e.g. Emotion or Styled Components), we have the ability to adjust styles using a "plugin" system (not unlike how PostCSS does this with modules). Ideally, the CSS-in-JS solution within Gutenberg would have this setup, so that we don't have to manually do things when we need to write styles. An automated solution may look similar to the one I've added for G2 Components: Note: It's using CSS Janus, one of the go to solutions for RTL conversion. Unfortunately, it's not GPL compatible... so we'll need to figure something out there 🙃. I'd try to write one myself, but my REGEX game is weak sauce 😂 . 🤷♀️ So, what's to do?Since this auto adjustment mechanic is missing... I've added an It gets us over the hump of not having RTL handling at all. However, you can imagine it's quite cumbersome and "verbose" - not too pleasant from a developer experience perspective. Hopefully this helps! ❤️ I can help submit some code tomorrow with the (It was a simple and naive solution. Apologies! 🙏 ) |
|
@saramarcondes Thanks for the explanation, I don't think an issue is needed, I was just curious and this is one of the main reasons we decided to use CSS-in-JS in components so I think these efforts are good in general. Aside: It seems the bundle size check only works for folks with the rights in the repo (not for fork PRs) and I'm curious to see the impact of these PRs. I don't expect it to create a big difference but just want to at least have an idea. |
@youknowriad Any suggestions for how we can address that? |
|
@ItsJonQ I took a shot at trying to make this RTL safe, maybe you can review and let me know if I'm on the right track? |
|
|
||
| ${ reduceMotion( 'animation' ) } | ||
|
|
||
| transform-origin: ${ ( p ) => p.yAxis } ${ getRTLSafeXAxis }; |
There was a problem hiding this comment.
@saramarcondes Haii! It looks like you have the RTL figured out :).
My suggestion here would be to abstract the transform-origin logic into another function.
My thinking for that is because having all inline is tricky to read (at least for me).
Abstracting it will allow us to write something like this:
${ appearTransformOrigin };
(or something similar)
And the prop interpolation + RTL would be handled in that :)
The same could apply for the SliderWrapper.
P.S. I found that outputting the styles in the abstracted function works best using the css function from emotion, example:
It seems to play nicer with editors, prettier, and linters.
Note: It could be used with string interpolation or as a function css({ ...}).
<3
|
@ItsJonQ I left a comment for you! Thanks again for your efforts 🙏 |
There was a problem hiding this comment.
🚀 from me! Thank you for working on this @saramarcondes (and for following up with the suggestions!) <3
|
Closing this PR as this approach is no longer possible given the recent refactor of this component in #26063 |
Description
This PR updates the
Animatecomponent to use CSS-in-JS similar to how newer components do like theAnglePickerControlandText.How has this been tested?
I've compared my changes in storybook to the production storybook and confirmed there are no behavioral or visual differences.
Types of changes
This might introduce a breaking change in that the old classnames will no longer appear on the animated component–however it seems to me like it would be an anti-pattern to target generated classnames rather than adding ones own classnames, so I think we should accept this (if it is indeed a breaking change).
This also deprecates the
middlevalue for theyAxispart oforiginfor theappeartype of animation. I chose to do this because it simplified the logic for selecting the correct Y and X axis oftransform-originwhile also getting rid of a non-standard value fororiginthat wasn't even being used. Now any validtransform-originvalue is actually going to be used correctly when passed asorigin. I thought for a minute about removing the little parsing logic we do; however, this logic maintains some specific default values for the Y and X axis so I chose to keep it in.Otherwise this PR maintains the ability for children to be a function and keeps the
classNameargument null-safe.Checklist: