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

[styles] Document the CSS prefixing strategy on the server #14139

Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 10, 2019

Switch to standard CSS properties wherever the same values are used. jss-plugin-vendor-prefixer will do the rest.

@eps1lon eps1lon added core component: button This is the name of the generic UI component, not the React module! component: link This is the name of the generic UI component, not the React module! labels Jan 10, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 10, 2019

Did you check the git history? I think that we should add a comment but keep this prefixes.

@eps1lon
Copy link
Member Author

eps1lon commented Jan 10, 2019

Sure did. It was added in #10063.

In the same PR the .browserslistrc was added which led me to believe that that was the actual issue with appearance.

It should still result in CSS with the required prefixes in the client and on the server (looks good on https://deploy-preview-14139--material-ui.netlify.com/demos/buttons/). The PR didn't mention why we need to add the prefix in this case so if there is a reason we should add a comment. Otherwise this looks fine to me now since no regression test failed.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 10, 2019

@eps1lon From what I recall, they were two issues. One important, one less. The important issue is that JSS doesn't do prefixes on the server. inputs and buttons without the appearance prefix looks really bad. The second issue what with the error reported by https://validator.w3.org/.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 10, 2019

Here the first paint on Chrome without the prefix when doing SSR:

capture d ecran 2019-01-10 a 14 12 11

@eps1lon
Copy link
Member Author

eps1lon commented Jan 10, 2019

JSS doesn't do prefixes on the server

  1. Let's assume that this is true: Why not prefix e.g. transform?
  2. It does if it knows the supported browsers which is guarenteed with .browserslistrc. Just view the source of e.g. https://deploy-preview-14139--material-ui.netlify.com/demos/buttons/ and search for "appearance" it will appear with moz and webkit prefix.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 10, 2019

Let's assume that this is true: Why not prefix e.g. transform?

@eps1lon It's a tradeoff, adding the appearance is a low overhead fix that prevents layout jumps on Chrome, latest version. Same problem with the old browsers and the flex prefixes.

@eps1lon
Copy link
Member Author

eps1lon commented Jan 10, 2019

Let's assume that this is true: Why not prefix e.g. transform?

@eps1lon It's true.

I just proofed to you with my second point that it's not. Where did you get the screenshot from? https://deploy-preview-14139--material-ui.netlify.com/demos/selects/ doesn't look like this with disabled javascript.

Also the source you linked is from an alpha package that introduces a breaking change over our current documented solution.

It's a tradeoff, adding the appearance is a quick fix that prevent layout jumps.

Based on what? I always here this "opinionated" argument but it seems like this is only used when it's convenient. How do you ensure that (if this would be the case) other occurrences of non-prefixed css props don't cause a layout jump as well?

I'm ok with adding a note to the docs that you should pipe the css through autoprefixer until cssinjs/jss#279 is resolved but right now this inconsistency was introduced without an argument for it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 10, 2019

@eps1lon You can't use the material-ui documentation as a baseline here. We are using https://autoprefixer.github.io/ to work around the issue in the documentation. I doubt a lot of our users use a server side CSS prefixer given the performance overhead and the extra time needed to set this up. At onepixel, we only use it for cached requests.

Based on what? I always here this "opinionated" argument but it seems like this is only used when it's convenient. How do you ensure that (if this would be the case) other occurrences of non-prefixed css props don't cause a layout jump as well?

Yes, it's opinionated, it's based on the idea that Chrome latest version market share is x10 of what the browsers that don't support flex prefixes have.

@eps1lon
Copy link
Member Author

eps1lon commented Jan 10, 2019

@eps1lon. I doubt a lot of our users use a server side CSS prefixer given the performance overhead and the extra time needed to set this up.

Based on what? I always here this "opinionated" argument but it seems like this is only used when it's convenient. How do you ensure that (if this would be the case) other occurrences of non-prefixed css props don't cause a layout jump as well?

Yes, it's opinionated, it's based on the idea that Chrome latest version market share is x10 of what the browsers that don't support flex prefixes have.

I don't follow the argument that reduces vendor prefixes to flexbox support and chrome. Did some more research and it seems like that no browser support for the appearance standard was the reason. Adding a minor bundle size overhead in that case.

I would move to an additional note about browser support on the server. It's currently not obvious that some features related to CSS might not be supported for server side rendering.

@oliviertassinari
Copy link
Member

I'm happy to see Gatsby doing it :). What makes you think using next-css makes autoprefixing easy?
What about the people that don't want to do server-side prefixing because it's slowing down the rendering for dynamic pages (like Onepixel)?

I would move to an additional note about browser support on the server. It's currently not obvious that some features related to CSS might not be supported for server side rendering.

I agree, I think that it would be great to document cssinjs/jss#279.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 10, 2019

Regarding the flexbox, I believe that doing the prefixes by hand is x10 more costly than what we have for the appearance as it's used more often and the prefixes aren't trivial. Flexbox yields x10 less value as affect way fewer browsers. We can extend this logic to the other CSS properties we need to prefix that impact might be noticed between the first server side paint and the client side rehydration.
So overall, if we do a value/cost matrix, I think that the appearance has a really interesting figure. I couldn't find any other CSS property with that compelling figure.

@eps1lon
Copy link
Member Author

eps1lon commented Jan 10, 2019

What makes you think using next-css makes autoprefixing easy?

I retract that statement. Have to make a lot of assumptions and with the current landscape of styling solution I would need to do some more research. It just "seemed" that way since they had postcss usage in their readme but I don't know how this applies to every styling solution.

@eps1lon eps1lon force-pushed the feat/core/leverage-jss-vendor-prefixer branch from aa20abc to 0943473 Compare January 10, 2019 15:43
@eps1lon eps1lon force-pushed the feat/core/leverage-jss-vendor-prefixer branch from 0943473 to 4d1dd4d Compare January 10, 2019 16:35
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation PR: good for merge package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. and removed component: button This is the name of the generic UI component, not the React module! component: Input component: link This is the name of the generic UI component, not the React module! labels Jan 10, 2019
@oliviertassinari oliviertassinari changed the title [core] Leverage jss-vendor-prefixer [styles] Document the CSS prefixing strategy on the server Jan 10, 2019
@oliviertassinari oliviertassinari merged commit b1769fa into mui:master Jan 11, 2019
@eps1lon eps1lon deleted the feat/core/leverage-jss-vendor-prefixer branch January 11, 2019 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants