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

[styled-engine] Add StylesProvider with injectFirst option #23934

Merged
merged 25 commits into from Dec 14, 2020

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Dec 10, 2020

This PR adds StylesProvider with injectFirst option in both @material-ui/styled-engine and @material-ui/styled-engine-sc. The StylesProvider is exported from core as StyledEngineProvider and used in the docs, codesandbox example template & the argos TestViewer.

The usage of this Provider will be necessary during the transition stage, where we have emotion components being overridden by JSS styles.

  • Find issue with @material-ui/styled-engine-sc's generated style tag being ignored
  • Find issues with remaining argos differences 😕

@vercel
Copy link

vercel bot commented Dec 10, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mui-org/material-ui/dzfp3vav0
✅ Preview: https://material-ui-git-feat-styled-engine-prepend.mui-org.vercel.app

@mnajdova mnajdova changed the title Feat/styled engine prepend [styled-engine, styled-engine-sc] Add StylesProvider with injectFirst option Dec 10, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Dec 10, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 435e8e5

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Find issues with remaining argos differences 😕

Capture d’écran 2020-12-10 à 18 01 53

  • This one is linked to the <Box component={SvgIcon} /> pattern. Emotion does no longer win specificity with font-size. I guess it's an acceptable cost during the migration

Capture d’écran 2020-12-10 à 18 02 07

Capture d’écran 2020-12-10 à 18 02 07

  • This one seems to be the same issue. clone does no longer work. We could also say that it's fine considering it will be resolved once the migration is completed.

  • The last one, with the drawer. The size of the drawer increase by 1px, the width of the border. It's box-sizing related. The regression tests are supposed to set boxSizing: 'content-box',. However, it's only now that it takes effect. Looks like a fix if you ask me. However, it also surfaces that these demos are not box-sizing invariant, not great. I would encourage we update the demos to force the box-sizing to border-box.

packages/material-ui/src/index.js Outdated Show resolved Hide resolved
packages/material-ui-styled-engine/package.json Outdated Show resolved Hide resolved
Comment on lines 9 to 13
const head = document.head;
injectFirstNode = document.createElement('style');
injectFirstNode.setAttribute('data-styled', 'active');
injectFirstNode.setAttribute('data-styled-version', '5.2.1');
head.insertBefore(injectFirstNode, head.firstChild);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Should we cleanup the style node after introduction?
  • Did you consider <StyleSheetManager target={}> with a comment node as target?

Provide an alternate DOM node to inject styles

https://styled-components.com/docs/api#stylesheetmanager

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it at some point but it didn't work, but it may have been related to other changes. Let me try again, it should definitely be cleaner than this. Also this doesn't work at the moment, the node is created but the styles are being added to a different node.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found out why it wasn't working, but it is awkward. The node added has to be style tag with data-styled='active'. Adding just a comment doesn't work. Basically, now it works, but we have first empty style tag, and then the one from styled-components.. Will leave it like this until we find a better alternative..

…ovider.js

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 10, 2020

Should we do this for all demos? Like a global style? Or explicitly for the failing demos?

@mnajdova We apply box-sizing: content-box; in the visual regression tests and box-sizing: border-box; in the documentation to spot the few lines of CSS that aren't invariant.

https://github.com/mui-org/material-ui/blob/d3cfb6bca523eaa58b768b6371b07009073d7aaa/test/regressions/TestViewer.js#L11-L19

I was suggesting we do the same thing as with the core components but for the demos that are impacted, e.g.

diff --git a/docs/src/pages/components/drawers/ResponsiveDrawer.tsx b/docs/src/pages/components/drawers/ResponsiveDrawer.tsx
index 9158b08af2..b4ddd17624 100644
--- a/docs/src/pages/components/drawers/ResponsiveDrawer.tsx
+++ b/docs/src/pages/components/drawers/ResponsiveDrawer.tsx
@@ -45,6 +45,7 @@ const useStyles = makeStyles((theme: Theme) =>
     toolbar: theme.mixins.toolbar,
     drawerPaper: {
       width: drawerWidth,
+      boxSizing: 'border-box',
     },
     content: {
       flexGrow: 1,

Most of the time, it works fine.

@mnajdova
Copy link
Member Author

@oliviertassinari did the changes the only differences now are the ones which we could consider as an acceptable cost during the migration. ( We can update the examples for the Box to override another component where we won't have conflicts with the styles being applied, like Typography maybe... ).

Regarding the migration guide, should I update the migration-v4.md with instruction that developers should use the StylesProvider while migrating, so that JSS overrides could win over the emotion default styles?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! Hopefully, thats it, we have covered most of the challenges with the migration to emotion.

@oliviertassinari oliviertassinari added the package: styled-engine Specific to @mui/styled-engine label Dec 11, 2020
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 13, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 14, 2020
@mnajdova mnajdova merged commit d420e77 into mui:next Dec 14, 2020
@oliviertassinari oliviertassinari changed the title [styled-engine, styled-engine-sc] Add StylesProvider with injectFirst option [styled-engine] Add StylesProvider with injectFirst option Dec 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styled-engine Specific to @mui/styled-engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants