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

@material-ui/styles #13503

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 3, 2018

This is a proof of concept. I have taken many shortcuts to make everything works together. This pull request is introducing:

  • An isolated @material-ui/styles package. I think that we can start with a v3.0.0-alpha.0 release.
  • Official support for the hook API.
  • a CSS = f(props) feature like all the major CSS-in-JS solutions have.
  • Use the new context API.
  • Upgrade to React v16.7.0-alpha.0.
  • Official support for the styled-components API
  • Official support for the render prop API

capture d ecran 2018-11-04 a 00 46 28

Closes #7633
Help with #13465

Merging this pull request will take some time. @mui-org/core-contributors early feedback is welcomed :)

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 4, 2018

@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-

@jrmyio
Copy link

jrmyio commented Nov 4, 2018

Official support for the styled-components API.

Would love to see an example of this!

Copy link
Member

@eps1lon eps1lon left a 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.

packages/material-ui-styles/src/createStyles.js Outdated Show resolved Hide resolved
packages/material-ui-styles/src/index.js Outdated Show resolved Hide resolved
packages/material-ui-styles/src/withStyles.js Show resolved Hide resolved
packages/material-ui-styles/src/withTheme.js Show resolved Hide resolved
"author": "Material-UI Team",
"version": "3.0.0-alpha.0",
"description": "Material-UI Styles - The styling solution of Material-UI.",
"main": "./src/index.js",
Copy link
Member

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.

packages/material-ui/src/styles/withStyles.js Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@sakulstra
Copy link
Contributor

sakulstra commented Nov 4, 2018

Sorry for this ot question:

It's not about using styled-components under the hood (we will see later for that)

@oliviertassinari Is this an ongoing discussion somewhere? I'd love to read the arguments for both sides

@oliviertassinari oliviertassinari added this to the v3.5.0 milestone Nov 5, 2018
@oliviertassinari oliviertassinari self-assigned this Nov 8, 2018
@oliviertassinari oliviertassinari force-pushed the with-styles-new-context branch 13 times, most recently from 34953bd to e48f140 Compare November 11, 2018 23:02
@oliviertassinari
Copy link
Member Author

Is this an ongoing discussion somewhere? I'd love to read the arguments for both sides

@sakulstra The styled components API implementation is less than 20 lines of codes with the withStyles higher-order components. It's the simplest solution.

@oliviertassinari oliviertassinari force-pushed the with-styles-new-context branch 2 times, most recently from 631d520 to fced972 Compare November 11, 2018 23:07
@oliviertassinari oliviertassinari merged commit 83adb95 into mui:master Nov 12, 2018
@oliviertassinari oliviertassinari deleted the with-styles-new-context branch November 12, 2018 18:25
@nicholas-l
Copy link
Contributor

nicholas-l commented Nov 12, 2018

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?
Would this be better:

const themeContext = React.useContext(ThemeContext);
const theme = listenToTheme ? themeContext : noopTheme;

Or is it OK because listenToTheme a constant for the life of the function?

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Nov 12, 2018

@nicholas-l Yes, it's fine. It will never change between two renders. Thank you for checking the implementation out.

@franklixuefei
Copy link
Contributor

What about type definition updates? :-D

@eps1lon
Copy link
Member

eps1lon commented Nov 13, 2018

@franklixuefei Need to finish up the context API rewrite for FormControl today and then I'll take a look.

@jrestall
Copy link

// index.tsx

import { install } from '@material-ui/styles';

install();

// Ensures install is called before anything else.
// We can remove this once Material-UI v4 is released
require('./init.tsx') // Standard Index.tsx code

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
Copy link

XHMM commented Nov 19, 2018

got An unexpected error has occurred. when visiting https://material-ui.com/css-in-js/basics/
and the console error is:
image

@oliviertassinari
Copy link
Member Author

@XHMM It hasn't been translated yet. You can change the language:

capture d ecran 2018-11-19 a 09 17 18

@yhaiovyi
Copy link

It doesn't really solve the problem doesn't it? What if I need to change not root styles?

@oliviertassinari
Copy link
Member Author

@yhaiovyi What do you mean?

@yhaiovyi
Copy link

Every example operates with { root: ... } styles only, and you inject it in element through className, which is confusing the least to say.

@yhaiovyi
Copy link

yhaiovyi commented Mar 25, 2019

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?

@oliviertassinari
Copy link
Member Author

@yhaiovyi You can use the prop in the style: https://next.material-ui.com/css-in-js/basics/#adapting-based-on-props.

@yhaiovyi
Copy link

Doesn't explain styling of nested elements, none of examples covers that topic.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Mar 25, 2019

@yhaiovyi Something like this?

withStyles({
  tooltip: {
    margin: props => props.size === 'large' ? 20 : 10,
  },
})(({ size, ...props }) => <Tooltip {...props} />);

@yhaiovyi
Copy link

Damn, I may be doing something wrong, I was sure I've tried that way. But hold on, so should it work with 3.9.2 ?

@oliviertassinari
Copy link
Member Author

@yhaiovyi No, v4.

@yhaiovyi
Copy link

And can you think about any ETA for at least beta? Thanks.

@oliviertassinari
Copy link
Member Author

@yhaiovyi We will have beta once #13663 is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants