Utility classes #3
Conversation
mnajdova
commented
Jul 15, 2020
mnajdova
commented
- I have followed (at least) the PR section of the contributing guide.
| cssProperties.forEach((cssProperty) => { | ||
| spacings[cssKey][ | ||
| `${properties[property]}${cssProperty}` | ||
| ] = theme.spacing(val / 2); |
There was a problem hiding this comment.
Later on, we will probably want to move mui#21030 forward.
| @@ -153,6 +153,7 @@ const pages = [ | |||
| { pathname: '/system/screen-readers' }, | |||
| { pathname: '/system/typography' }, | |||
| { pathname: '/system/api', title: 'API' }, | |||
| { pathname: '/system/global-classes' }, | |||
There was a problem hiding this comment.
Seeing the structure makes me think of how we don't explain well the value proposition of the helpers on the /basics page. http:https://material-ui.com/system/basics#real-world-use-case should be at the top like they did on https://tailwindcss.com/docs/utility-first. Or even https://chakra-ui.com/ on the homepage 😆
There was a problem hiding this comment.
I was missing explanation a bit too to be honest..
| } | ||
| }; | ||
|
|
||
| export default function colors(theme) { |
There was a problem hiding this comment.
I wonder if it would make sense to expose the Material Design colors https://material-ui.com/customization/color/#2014-material-design-color-palettes
There was a problem hiding this comment.
Interesting, I just exported only the ones that are in the palette inside the theme, thinking that if users will restrict that set or add new colors the classes would reflect that. On the other hand I don't see why we should not expose these colors too...
There was a problem hiding this comment.
If we have a doubt, we can wait to see if people ask for it
| whiteSpaces, | ||
| } from './displays'; | ||
|
|
||
| const GlobalCss = withStyles((theme) => { |
There was a problem hiding this comment.
I think that we will need to look at
- the time it takes to build the styles
- how we can handle server-side rendering, if we trust https://tailwindcss.com/docs/controlling-file-size/, 144.5kb gzipped is no way for inlining critical CSS. In our current experimentation, we are at 4kb gzipped. https://www.nytimes.com/ is 210kB gzipped. Youtube.com is 56kb gzipped. Amazon.com is at 93kb gzipped. So I would say 40kB gzipped isn't too far from our limit.
There was a problem hiding this comment.
Actually, for 2. I remember emotion having some sort of critical extraction tool, to only include the required class names. Tailwind has something similar, if it's fast enough to be run at each server-side query, it could move the limitation to 1. only. On the server 1. can be cached.
There was a problem hiding this comment.
For 1. I measure 1ms to build the object and 12ms for JSS to process the object before injection in the page. I have no idea how it scales. They might be way to bypass the plugins, speeding things up. We would also need to look at how long it makes with styled-components and emotion.
| '.d-none': { display: 'none' }, | ||
| '.d-initial': { display: 'initial' }, | ||
| '.d-inherit': { display: 'inherit' }, | ||
| '.d-print-inline': { '@media print': { display: 'inline' } }, |
There was a problem hiding this comment.
I wonder if we couldn't handle print as a custom breakpoint, as we would handle sm, or md.
There was a problem hiding this comment.
One thing I was thinking about was, whether we should include the breakpoints inside the class names (Vuetify has them), so that.. It could be useful I think
There was a problem hiding this comment.
Yeah, it seems that being able to change the values based on the media query is a critical feature they all have. I also don't see how I could implement anything without it 😁. And yes, agree, to make it work, it has to be in the class names.
The question I see left is where? Should it be a prefix like Tailwind, or in the middle Like Bootstrap/Vuetify, or should it be at the end? I think that it's mostly about how you want to organize your code. Consider how the sx prop and our current system works, I would be leaning toward as a prefix sm:p-3, print:p-3. Thought?
There was a problem hiding this comment.
Hum, not sure actually, we document the following for the props API:
display={{ xs: 'none', sm: 'block' }}
There was a problem hiding this comment.
added breakpoints on all classes as prefix
There was a problem hiding this comment.
I left print as is for now, mainly because it may be combined with other breakpoints...
There was a problem hiding this comment.
- What's missing before we can accurately know the performance implication of the approach?
- Regarding VSCode, we could imagine the same extension as https://marketplace.visualstudio.com/items?itemName=Zignd.html-css-class-completion but for Material-UI, close to what @jedwards1211 did in https://github.com/vscodeshift/material-ui-snippets
- 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?
- Shouldn't we move the pull request to the main repo as a draft?
| '.d-none': { display: 'none' }, | ||
| '.d-initial': { display: 'initial' }, | ||
| '.d-inherit': { display: 'inherit' }, | ||
| '.d-print-inline': { '@media print': { display: 'inline' } }, |
There was a problem hiding this comment.
Hum, not sure actually, we document the following for the props API:
display={{ xs: 'none', sm: 'block' }}
|
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 |
Co-authored-by: Olivier Tassinari <[email protected]>
|
@oliviertassinari here are some answers.
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.
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?)
Let me move the changes to the system package and will create draft PR on the main repo 👍 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... @oliviertassinari what are your thoughts? |
