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

[system] Introduce utility classes #21815

Closed
wants to merge 23 commits into from

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Jul 17, 2020

This PR is experimenting with generating utility classes that can be used on all components instead of the system properties. It is heavily inspired by the Tailwind classes.

Previous discussions: mnajdova#3

@mnajdova
Copy link
Member Author

@oliviertassinari sorry for the misdirection, let's continue the discussions here.

What's missing before we can accurately know the performance implication of the approach?

We were missing the breakpoints, they are added now, so I believe we should be ready. I was not sure whether we want to generate classes for the borders, if we do we may want to add those first too.

Regarding the diff with the prop base approach. I assume people will only want to use one of the two approaches, depending on what they enjoy most. I wonder about how we can best organize the documentation have the two live together. What elements we should highlight to help developers pick a side?

Hmm, I was ideally thinking that we would create the utility classes which can be used on all components, so that we don't need to actually add the Box's system API on all components. If I am correct in this thinking, I would say that the Box should be use only for prototyping purposes, not sure when we would like to use it over the utility classes (maybe if people need more dynamics?)

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 17, 2020

@material-ui/lab: parsed: 0.00% 😍, gzip: +0.34%
@material-ui/system: parsed: +284.85% , gzip: +310.52%

Details of bundle changes

Generated by 🚫 dangerJS against 09bce37

@mnajdova mnajdova changed the title Feat/utility classes [system] Introduce utility classes Jul 17, 2020
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.

Great to see further exploration in this direction

I wasn't thinking, these CSS classes are defined in JS files. Maybe we could propose a way to have that extension run user-defined scripts that return additional class names...seems like it wouldn't be super trivial to make our own thing from scratch, I haven't looked at what technique they use to decide when to show the autocomplete

@jedwards1211 Another VS code extension we could benchmark against is https://marketplace.visualstudio.com/items?itemName=bradlc.vscode-tailwindcss.

Hmm, I was ideally thinking that we would create the utility classes which can be used on all components, so that we don't need to actually add the Box's system API on all components. If I am correct in this thinking, I would say that the Box should be use only for prototyping purposes, not sure when we would like to use it over the utility classes (maybe if people need more dynamics?)

@mnajdova From my perspective, the Box component has no purpose if the end goal isn't to allow the support of the props in all the components. With class utilities, what's the use case for a Box only component? I don't see it. I think that this discussion can be converted into:

  • Bring clarity with an RFC, ask for feedback from the community
  • Structure the documentation to explain why a developer reading the page should care about utility classes. Guide the developers in making the tradeoffs between using global class names and the style props.

I also wonder about: do we need utility classes if we can bring something equivalent to the css prop of emotion with an extra transformation layer to leverage the design tokens present in the theme?

@@ -0,0 +1,22 @@
import { withStyles } from '@material-ui/styles';
Copy link
Member

Choose a reason for hiding this comment

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

⚠️ We need to consider how we want to organize the dependency tree.

import positions from './positions';
import displays from './displays';

const GlobalCss = withStyles((theme) => {
Copy link
Member

@oliviertassinari oliviertassinari Jul 17, 2020

Choose a reason for hiding this comment

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

continuing the discussion on mnajdova#3 (comment)

We are now at this point (production?):

  1. style generation: 10ms (OK)
  2. style injection: 180ms (hum)
  3. payload: 20kB gzipped (hum)

A couple of ideas around how we can improve performance:

  • make the print media query optional (I think that most people don't care)
  • remove !important
  • work with JSS around how we can skip some of the plugins.

A couple of things to explore:

  • What's the performance implication with styled-components?
  • What's the performance implication with emotion?
  • What will happen when the developers ask for the support of more CSS properties? e.g. all the CSS grid and flexbox properties? How will it scale? My biggest concern is with 2. as (1. seems OK and 3. can ben edged with a pruning logic, at least with JSS. But is it true with emotion or styled-components that try to hydrate the CSS too? Probably not.)

Copy link
Member

@oliviertassinari oliviertassinari Jul 17, 2020

Choose a reason for hiding this comment

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

A benchmark of some of the top Alexa websites, to give an idea of how far we can go:

  • nytimes.com: 210kB gzipped, 1,667ms of JS
  • youtube.com: 56kB gzipped, 11,440ms of JS
  • amazon.com: 93kB gzipped, 1,562ms of JS
  • wikipedia.com (random page): 36kB gzipped, 626ms of JS

Another consideration is that these utility classes are required to display the content, they need to resolve early on. Which in my above benchmark accounts for all JavaScript loading (excluding analytics & ads)

@sakulstra
Copy link
Contributor

I've just read trough this pr and I think the approach is super cool 👍 🌈 (was wondering if it would it be an option to use tailwind instead of rebuilding parts of it though?)

Note: I had to add !important to some of the classes generate, to make sure they would always win, not sure if it is the best approach...

!imporant is essentially a no-go for pages that target amp so that's sth that needs to be solved.

@mnajdova
Copy link
Member Author

mnajdova commented Jul 19, 2020

!imporant is essentially a no-go for pages that target amp so that's sth that needs to be solved.

@sakulstra I meant to update the PR to remove the !important, but didn't manage so far. It is updated now 👍

was wondering if it would it be an option to use tailwind instead of rebuilding parts of it though?

This is a great point! I am still mostly investigating around this, there has to be an option for clients to whitelist/blacklist which props to be generated, so will definitely think about this option.

@oliviertassinari
Copy link
Member

Should we close the pull request? It seems that in #22819 we are moving away from this class name utility-based approach.

@mnajdova mnajdova closed this Oct 5, 2020
@zannager zannager added the package: system Specific to @mui/system label Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants