-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
@material-ui/styles #13503
@material-ui/styles #13503
Conversation
@ConneXNL It's not about using styled-components under the hood (we will see later for that), it's about publishing this documented API in a module: https://material-ui.com/customization/css-in-js/#styled-components-api---15-lines- |
Would love to see an example of this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm always in favor of leveraging the monorepo pattern more. I'm not familiar with the styling implementation so I'm not able to spot fundamental changes. Overall this looks fantastic apart from some minor nitpicks.
One thing I already started locally was creating a scripts package for stuff like test runners and build tasks which are currently duplicated throughout the repo. This will only get worse with every package we add.
"author": "Material-UI Team", | ||
"version": "3.0.0-alpha.0", | ||
"description": "Material-UI Styles - The styling solution of Material-UI.", | ||
"main": "./src/index.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should point to build/index.js
.
Sorry for this ot question:
@oliviertassinari Is this an ongoing discussion somewhere? I'd love to read the arguments for both sides |
34953bd
to
e48f140
Compare
@sakulstra The styled components API implementation is less than 20 lines of codes with the |
631d520
to
fced972
Compare
fced972
to
f6ed058
Compare
Isn't have the ternary conditional (const theme = listenToTheme ? React.useContext(ThemeContext) : noopTheme;) in makeStyles bad? I thought you couldn't have hooks in conditionals? const themeContext = React.useContext(ThemeContext);
const theme = listenToTheme ? themeContext : noopTheme; Or is it OK because listenToTheme a constant for the life of the function? |
@nicholas-l Yes, it's fine. It will never change between two renders. Thank you for checking the implementation out. |
What about type definition updates? :-D |
@franklixuefei Need to finish up the context API rewrite for |
// index.tsx
I'm doing this to ensure install is called first and to therefore avoid conflicting jss class names when building for production. Is there a recommended cleaner approach for now? |
@XHMM It hasn't been translated yet. You can change the language: |
It doesn't really solve the problem doesn't it? What if I need to change not root styles? |
@yhaiovyi What do you mean? |
Every example operates with { root: ... } styles only, and you inject it in element through className, which is confusing the least to say. |
Btw, thanks that quick response. But speaking of the real world example. I need to be able to set offsets for Tooltip based on component's props. If I wanted them static, I'd do something like withStyles({ tooltip: { margin: '10px' }})(Tooltip) but what should I do, if I need to set that margin with property? And the last question is that possible to have that feature with mui stable, or alpha is the only option? |
@yhaiovyi You can use the prop in the style: https://next.material-ui.com/css-in-js/basics/#adapting-based-on-props. |
Doesn't explain styling of nested elements, none of examples covers that topic. |
@yhaiovyi Something like this? withStyles({
tooltip: {
margin: props => props.size === 'large' ? 20 : 10,
},
})(({ size, ...props }) => <Tooltip {...props} />); |
Damn, I may be doing something wrong, I was sure I've tried that way. But hold on, so should it work with |
@yhaiovyi No, v4. |
And can you think about any ETA for at least beta? Thanks. |
This is a proof of concept. I have taken many shortcuts to make everything works together. This pull request is introducing:
@material-ui/styles
package. I think that we can start with av3.0.0-alpha.0
release.CSS = f(props)
feature like all the major CSS-in-JS solutions have.Closes #7633
Help with #13465
Merging this pull request will take some time. @mui-org/core-contributors early feedback is welcomed :)