Closed oliviertassinari closed 4 years ago
einazare
commented
6 years ago Hello @oliviertassinari ,
Thank you for getting back this quick with a response/feedback to our product. I've read all of your points and i agree with them. I'm not sure if i'll have time this week to make these changes. If i won't have the time this week, then i'll make an update next week. I hope this is ok with you.
Manu
oliviertassinari
commented
6 years ago I haven't had the time to look at everything in detail yet. Give me a week or two.
mbrookes
commented
6 years ago to be cautious with:
import { BugReport, Code, Cloud } from 'material-ui-icons';Similarly with the recommendation here: https://creativetimofficial.github.io/material-dashboard-react/#/documentation/styles
See: https://material-ui-next.com/guides/minimizing-bundle-size/
einazare
commented
6 years ago Hello @oliviertassinari ,
Let me say that i'm sorry we haven't made the changes earlier, we've been a bit busy with other work. I will let you know in this comment what i've managed to implement from your feedback and make some comments alongside with what i couldn't done or understand and maybe you could give a more detailed description to those points that i haven't been able to understand.
[x] Use prettier?
[x] Missing documentation live website.
[x] 29,5% compression possible with the src/assets/img folder. I have used imageOptim to get this figure.
[x] classes.root is equivalent to using className.
[x] the classNames() helper can avoid doing brittle class name manipulation.
See: https://material-ui-next.com/guides/minimizing-bundle-size/ I do not know how to use any of these plugins from here. I tried some stuff, but none of it worked. For the moment we are going to leave the imports how they are (the code looks much cleaner this way) and i will try again to understand how to use one those plugins with react. I'm going to take a tutorial or something to understand better and maybe i'll be able to add one of the plugins.
Maybe we can use the spacing={16}
We need the padding to be set to 15 for consistency throughout our products (because on our other products - the html and angular version of this product - we use bootstrap, we have the padding set to 15 and we already have PSD and/or Sketch files which are the same for these products - the same PSD and/or Sketch files are used for both html and angular versions of this product and we want to use them for react version too - we don't want to create special files for react version).
The global override might not be needed with I do not understand what Reboot does - I'm sorry but i do not see the connection here, what Reboot does and what our css does (i will take another look at our css and your component). Maybe you can be a bit more explicit with Reboot?
[x] Try using function component over class component where possible, it's lighter.
[x] It seems you are no using the theme in this components
[x] You shouldn't need to explicit the prefixes. css-vendor use feature detecting to do it.
jss-default-unit is making a lot of unit optional
For now we are going to leave them like so. The problem is that we need to know where we use em, rem or px, it might confuse someone to see somewhere with rem or em, and than without any of them.
Using theme.zIndex.modal + 2 could be more explicit
We are going to leave these as they are the same in our other products.
[x] I was surprised to see all the styles in one file
Moved the styles variables in src/variables/styles.jsx and all the rest styles in src/variables/styles/{$nameOfStyle}.jsx files and import them where needed
I'm surprised there is no usage of the MuiThemeProvider
Totally agree with you here. When we started our work on this product we wanted to use MuiThemeProvider but could figure out how to use it, and went with the overriding styles :( . I will take a look again at this, maybe figure it out.
These changes will be here on github. We will wait for you feedback, and after that, we will upload the new version on creative-tim too.
Manu
oliviertassinari
commented
6 years ago I do not know how to use any of these plugins from here. I tried some stuff, but none of it worked. For the moment we are going to leave the imports how they are (the code looks much cleaner this way) and i will try again to understand how to use one those plugins with react. I'm going to take a tutorial or something to understand better and maybe i'll be able to add one of the plugins.
@EINazare The problem we are trying to solve is ensuring people don't end up loading the whole Material-UI library when they only need a subset of it. This can significantly increase the time to interaction for an application. The discussion has been pushed forward in https://github.com/mui-org/material-ui/issues/10212. I agree that it something that can be solved later on. The best solution to the problem is most likely the one used by react-router.
We need the padding to be set to 15 for consistency throughout our products
Sounds good.
I do not understand what Reboot does
You can find the style injected by Reboot here. It's simply for raising awareness. It's not necessarily the best approach, especially if you want to keep consistency with the other products.
jss-default-unit is making a lot of unit optional For now we are going to leave them like so. The problem is that we need to know where we use em, rem or px, it might confuse someone to see somewhere with rem or em, and than without any of them.
I agree it can definitely be confusing. It's following almost the same rules React use internally for the style property. Unitless values will either be replaced by px, % or ms based on the key name. Never rem or em. Again, I wanted to raised awareness. Doesn't mean you have to use it.
Using theme.zIndex.modal + 2 could be more explicit We are going to leave these as they are the same in our other products.
👍
MuiThemeProvider but could figure out how to use it, and went with the overriding styles :( . I will take a look again at this, maybe figure it out.
The MuiThemeProvider is allowing people to change some key values and be able to remove a lot of styles overrides. It can be linked to color, typography, spacing, media-queries, zindex. You can easily inspect the theme object in https://material-ui-next.com/customization/theme-default/.
einazare
commented
6 years ago Hello @oliviertassinari ,
Thank you again for coming back so quick with a response. We've discussed and we are going to move all our variables that we're using in different components (such as font-size, font-weight - typography - etc.) in a theme and add the MuiThemeProvider, and after we're going to release on creative-tim.com version 1.1.0.
Best, Manu
oliviertassinari
commented
6 years ago @EINazare As long as you see the value of such changes, go ahead. At the end of the day, pick what works best for you :). Thanks for your work!
Hi 👋 ,
I will gather my feedback on the project in this issue. You have done a great job so far! It's opening a new path for Material-UI. I want this project to be an inspiring theme example. I will invest time into it.
src/assets/imgfolder. I have used imageOptim to get this figure.classes.rootis equivalent to usingclassName. https://github.com/creativetimofficial/material-dashboard-react/blob/88ee44627534ff217bc8fce6e2e91d0835bfc0d3/src/components/CustomInput/CustomInput.jsx#L18-L20classNames()helper can avoid doing brittle class name manipulation. https://github.com/creativetimofficial/material-dashboard-react/blob/88ee44627534ff217bc8fce6e2e91d0835bfc0d3/src/components/CustomInput/CustomInput.jsx#L19spacing={16}property instead of: https://github.com/creativetimofficial/material-dashboard-react/blob/8e709abc4fc1357f38249300ffe3bf5fe537892a/src/components/Grid/ItemGrid.jsx#L6theme.zIndex.modal + 2could be more explicit https://github.com/creativetimofficial/material-dashboard-react/blob/e3ad7d53215e9f7ad056490ce6d3fed822881d26/src/variables/styles.jsx#L191MuiThemeProvider. For instance, setting the primary and secondary colors could have made the style override simpler.