Upgrade to Emotion 11 #32930
Merged
Upgrade to Emotion 11 #32930
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sarayourfriend
force-pushed
the
upgrade/emotion-11-sergio
branch
from
92e805b
to
c08951e
Compare
|
Size Change: +1.22 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
sarayourfriend
force-pushed
the
upgrade/emotion-11-sergio
branch
from
c08951e
to
05732a4
Compare
sarayourfriend
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
|
Hey @sarayourfriend , could you please rebase this PR? Merging #32763 and #32764 introduced some conflicts |
sarayourfriend
force-pushed
the
upgrade/emotion-11-sergio
branch
3 times, most recently
from
66cf0d7
to
9f5077c
Compare
gziolo
reviewed
Jun 24, 2021
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.
"@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.
gziolo
reviewed
Jun 24, 2021
| @@ -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.
That's a lot of gymnastics to make it work with Storybook :(
There was a problem hiding this comment.
Yeah it's unfortunate that this is necessary. Storybook is slow to upgrade 🤷
ciampo
reviewed
Jun 24, 2021
There was a problem hiding this comment.
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?
sarayourfriend
force-pushed
the
upgrade/emotion-11-sergio
branch
from
3b9bc28
to
7b17a6b
Compare
Nope, let's do that in a separate PR though. |
ciampo
requested changes
Jun 24, 2021
There was a problem hiding this comment.
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 |
ciampo
approved these changes
Jun 24, 2021
There was a problem hiding this comment.
I checked Storybook stories again and everything seems to work as expected. LGTM 🚀
I accidentally turned some places that imported css from
emotion/coreto import it fromemotion/cssinstead 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!!! |
30 tasks
|
FYI — looks like work has started on the Storybook side to update to Emotion 11 storybookjs/storybook#17640 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.jsfiles for terms that need renaming or removal).