-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Upgrade to Emotion 11 #32930
Upgrade to Emotion 11 #32930
Conversation
92e805b
to
c08951e
Compare
Size Change: +1.22 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
c08951e
to
05732a4
Compare
Hey @sarayourfriend , could you please rebase this PR? Merging #32763 and #32764 introduced some conflicts |
66cf0d7
to
9f5077c
Compare
packages/components/package.json
Outdated
"@emotion/cache": "^11.1.3", | ||
"@emotion/core": "^11.0.0", | ||
"@emotion/css": "^11.1.3", | ||
"@emotion/native": "^11.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@emotion/native"
is RN dependency and it should not be listed here because it pulls in additional dependencies that cause conflicts after publishing to npm. In general, we don't list RN dependencies for individual packages. It can stay listed as a dependency in the root package.json
file.
@@ -7,6 +12,24 @@ const stories = [ | |||
|
|||
const customEnvVariables = {}; | |||
|
|||
const modulesDir = path.join( __dirname, '../node_modules' ); | |||
|
|||
// Workaround for Emotion 11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a lot of gymnastics to make it work with Storybook :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's unfortunate that this is necessary. Storybook is slow to upgrade 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most changes are trivial: import renames and snapshot updates.
I also went through the Emotion migration docs and the issue/PR about the hack to make it work in Storybook, and left some notes based on my initial understandings.
Also, a we using the eslint rules for emotion ? If no, is there a reason why?
3b9bc28
to
7b17a6b
Compare
Nope, let's do that in a separate PR though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately some storybook stories have some differences from what's currently in production. Here are some of the differences that I could spot:
Story | Production | This PR |
---|---|---|
AlignmentMatrixControl |
||
BoxControl |
||
InputControl |
||
NumberControl |
||
RangeControl |
||
SelectControl |
There seems to be at least one common error in all stories: The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type"
Also, one of the major visual issues seems to be around input field borders.
It's strange, it appears those messages are from Emotion 10? emotion-js/emotion#1105 |
Okay, I'm able to reproduce the issue. The problem is that I accidentally turned some places that imported I just need to find all the places that used to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked Storybook stories again and everything seems to work as expected. LGTM 🚀
I accidentally turned some places that imported css from
emotion/core
to import it fromemotion/css
instead ofemotion/react
I guess that what I previously described as "trivial import renames" was not so trivial after all 😓 I should have not overlooked those changes at the start! Still, good thing that we caught it before merging this PR
@sarayourfriend , I guess we should open 2 follow-up PRs to:
- fix the error message
The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type"
, which seems to be caused by Storybook running the<Card />
story (not related to the issues described previously) - add the eslint rules for emotion
WDYT?
Yup, opening two new PRs for that sounds perfect! I can do that today 🎉 Thanks for the thorough review!!! |
FYI — looks like work has started on the Storybook side to update to Emotion 11 storybookjs/storybook#17640 |
Description
Upgrades to Emotion 11, includes a workaround for Storybook: storybookjs/storybook#13300 (comment)
How has this been tested?
Run Gutenberg with this PR and verify that Emotion components are still working as expected. Check that the StyleProvider is working as expected for iframes in FSE (enable a FSE enabled theme like TT1 Blocks and then verify that a block using Emotion components works correctly, for example the external link in the Google Calendar Jetpack block).
Also run storybook npm run storybook:dev and ensure that all Emotion components are working as expected.
Types of changes
Dependency update
Checklist:
*.native.js
files for terms that need renaming or removal).