Hide implementation details from styles API

[Izhaki]

So yesterday I was looking into the css in js section in the v1 docs, and equally delved into withRoot, and other examples. Amazing job.

But I suspect, having done so myself many times in the past, that you guys have put so much effort and thought into this, that you fell a step short from perfecting the API.

That said from a client perspective.

Current API

Currently, the examples involves:

import { withStyles, createStyleSheet } from 'material-ui/styles';

const styleSheet = createStyleSheet(theme => ({
  body: {
    margin: 0,
  },
}));

Followed by:

AppWrapper = withStyles(styleSheet)(AppWrapper);

Possible improvements

Now consider you achieve the same thing with:

import { applyStyleSheet } from 'material-ui/styles';

then (just an object)

const styleSheet = {
  body: {
    margin: 0,
  },
}

or (a function in case theme is needed)

const styleSheet = theme => ({
  body: {
    margin: 0,
  },
})

or (a tuple in case the sytlesheet name is important - the second parameter may be a function or an object)

const styleSheet = [ 'MyStyleName', theme => ({
  body: {
    margin: 0,
  },
})]

Followed by (given applyStyleSheet is curried):

return applyStyleSheet( styleSheet, MyComponent )

Basically, applyStyleSheet hides away withStyles and createStyleSheet, and has some switch logic to see if the style sheet provided is an object, function, or a tuple.

props.classes?

Lastly, another part of the API, as seen in your own doc's AppContent involves:

function AppContent(props) {
  const { className, classes, children } = props

  return (
    <div className={ classNames( classes.content, className ) }>
      {children}
    </div>
  )
}

withStyles injects a classes property on props.

It took a bit of mental mapping to understand that classes.content is actually the name of the class that is used in the stylesheet provided to withStyles.

It is nobody's fault ES6 has classes and CSS has classes.

I just wonder if using styleSheetClasses instead of classes, albeit more verbose, would be clearer? As in:

function AppContent(props) {
  const { className, styleSheetClasses, children } = props

  return (
    <div className={ classNames( styleSheetClasses.content, className ) }>
      {children}
    </div>
  )
}

[oliviertassinari]

I don't think that we should change the classes API.

Regarding the createStyleSheet API change suggestion, we can talk about it :).
If we can, I think that we should go for it, but the issue is with the implementation.

The question is around how we are going to merge with react-jss. Material-UI support some unique feature that react-jss might not want to support. Let's take the overrides theme key or the classes property as an example. How are we going to implement those feature outside of react-jss?

We gonna need a wrapper at some point.
The best case scenario is that we can hide it inside the withStyles higher-order component, the worse case is that we still need to put it under the createStyleSheet wrapper.

[oliviertassinari]

cc @kof

[oliviertassinari]

Also, this would be an important API change. I don't think that I will have the energy to go into, again, before v1. Maybe someone else might if we agree on a path going forward. I would hold that discussion until we move forward with react-jss.

[kof]

overrides sounds like a feature for theming

[Izhaki]

@oliviertassinari other concerns aside, what I'm proposing here is not a change to the API with regards to createStyleSheet.

All I'm suggesting is the addition of a small function - applyStyleSheet that hides away createStyleSheet and withStyles so code is clearer and there is less repetition and less imports needed.

I can write this function and submit it here if you wish - it should be 7 lines of code or so.

[oliviertassinari]

@Izhaki Right, but I don't think that we should add a applyStyleSheet function. That's going to increase the API surface. We have a potentially better alternative, make withStyles accept a GetStyles object. But if we go into that direction, why exposing createStyleSheet in the first place? That means we can remove it, and we should in order to minimize the API surface.
That's a large refactorisation.

If we make the assumption that no matter what we do with react-jss, we are going to keep our withStyles higher-order component, then we can go full steam into that direction and remove createStyleSheet.

[Izhaki]

That makes sense.

[Izhaki]

Hmm... just saw this on react-jss, which is exactly what I meant in my original post:

import React from 'react'
import injectSheet from 'react-jss'

const styles = {
  button: {
    background: props => props.color
  },
  label: {
    fontWeight: 'bold'
  }
}

const Button = ({classes, children}) => (
  <button className={classes.button}>
    <span className={classes.label}>
      {children}
    </span>
  </button>
)

export default injectSheet(styles)(Button)

[Izhaki]

@oliviertassinari

We have a potentially better alternative, make withStyles accept a GetStyles object. But if we go into that direction, why exposing createStyleSheet in the first place? That means we can remove it, and we should in order to minimize the API surface.
That's a large refactorisation.

If we make the assumption that no matter what we do with react-jss, we are going to keep our withStyles higher-order component, then we can go full steam into that direction and remove createStyleSheet.

Well, shouldn't the view be that material-ui styles is an extension of react-jss, achieved using a wrapper (that potentially leaves the API intact when possible)?

So is there a problem renaming withStyles to injectSheet, and hide createStyleSheet? I mean, as a developer, I'd be bloody happy to go to the react-jss docs and material-ui docs and see the same API - one less API to learn, and definitely more inviting to people already familiar with react-jss. Then spell your additions only.

Also, is this really such a large refactorisation?

[oliviertassinari]

@Izhaki I would avoid using the same wording as react-jss to avoid confusion. I think that we can go forward into that direction 👍 .

Also, is this really such a large refactorisation?

Could take 2-3 hours for the Material-UI codebase (yeah, 10 times less that what took me the style upgrade from alpha to beta) and then, users will have to upgrade their usage.

[Izhaki]

@oliviertassinari

I would avoid using the same wording as react-jss to avoid confusion

Not sure I agree with this, unless you foresee someone using react-jss alongside material-ui. What matters is what it does, and essentially, for the API consumer (like myself), withStyles does what injectSheet does.

ramda.js, lodash, underscore all have map and reduce functions - does the same thing so who cares which library it is from? (even though their API is actually not the same.)

But I guess there are pros and cons to each approach.

users will have to upgrade their usage

Luckily, you're still in pre-release phase. Having used the beta for a while now, I can't wait for the release, and given it being such a bounty, wouldn't mind any API changes.

[hubgit]

Once the need for users to call createStyleSheet is removed, it would also be helpful if withStyles could be exported from the main index, avoiding having to add the separate material-ui/styles import to every styled component.

import { withStyles } from 'material-ui'

const styles = theme => {
 // …
}

export default withStyles(styles)(Foo)

[Alxandr]

I've become a big fan of using function as child instead of HOC lately (just like react router does for instance). So I would really like if there was API so I could do the following (instead of withStyle):

const styles = /* create stylesheet somehow */;
const MyComp = () => (
  <Styled styles={styles}>
    {classes => (
      <div className={classes.someClass} />
    )}
  </Styled>
);

It took me a while to get used to the syntax (used to hate it, but that can be said about JSX in general), but when I figured out it's uses it's really nice. For instance, you don't tie people up in the prop being called classes. This does not prevent you from using any propname at all. And if you wanted to call it something else than classes, you could. The component being styled, and the styling component doesn't need to know about eachother at all, so the contract is smaller.

You can read more about it here: https://medium.com/merrickchristensen/function-as-child-components-5f3920a9ace9

[kof]

@Alxandr I was thinking about this interface too but haven't had time to decide on this. Ultimately having alternative interface is more harm than good so it needs to have a very serious reason besides of stylistic preferences or slightly smaller contracts. The biggest reason to start thinking about it was to provide a styling without HOC's because people often are incapable to understand HOC concept. The other thought is that HOC is part of react and people need to learn it.

[Alxandr]

@kof another thing to consider is that one can easily be implemented in terms of the other, yet implementing a "function-as-children" component based on a HOC is rather painfull.

// ignoring statics and all that jazz
const withStyles = (styles) => (BaseComponent) => {
  return class StyledComponent extends Component {
    render() {
      return (
        <Styled styles={styles}>
          {classes => <BaseComponent classes={classes} {...this.props} />}
        </Styled>
      );
    }
  };
};

[oliviertassinari]

I was thinking about this interface too but haven't had time to decide on this.

@kof While I find that the compound component API provides more flexibility over the Higher-order Component one, I also find the Higher-order Component API a game changer when needing to access the injected property in the life cycles. In our case, I think that the priority is the following "lifecycles > flexibility". Meaning I think that we should stick to the Higher-order component

ramda.js, lodash, underscore all have map and reduce functions

@Izhaki That's right, we can always change the wording. I can post a question on Twitter, we will see the result :).

import { withStyles } from 'material-ui'

@hubgit That makes sense, we can change that too. But I don't think that we should change the component location.

[oliviertassinari]

https://twitter.com/MaterialUI/status/894652180656992261

[Izhaki]

@oliviertassinari @hubgit I'm not sure about promoting withStyles to the root level of material-ui. Where do you draw the line?

I'm actually hiding MUI behind a module (so all app imports don't have material-ui directly, and I can extend component at will - a practice I always saw as beneficial and it lets you extend behaviour like adding throttle etc.) and for there's a clear separation between styles and controls.

[oliviertassinari]

@Izhaki We are already doing the following export { MuiThemeProvider } from './styles'; in the index.js. At least we need to be consistent. Should we change that too?

I'm actually hiding MUI behind a module

I have been doing the same on a large project, though on small ones I'm consuming it directly.

[Alxandr]

@oliviertassinari what do you mean you can access it in the lifecycles in a HOC and not with function as child?

class MyComp extends Component {
  constructor(props) {
    super(props);

    console.log(this.props.styles);
  }

  // and all the others
  onComponentDidMount() {
    console.log(this.props.styles);
  }
}

const App = () => (
  <Styled styles={myStyles}>
    {styles => <MyComp styles={styles} />}
  </Styled>
)

[Edit]
Please do remember that when you use a HOC, normally you end up with the following React render tree:

<jss(MyComp)>
  <MyComp />
</jss(MyComp)>

Which is exactly the same as you end up with if you have function as children. It's just how you get there that differs. There is no difference in lifecycle methods or any such (at least as far as I'm aware off, if I'm wrong please let me know).

[oliviertassinari]

@Alxandr We won't go into this solution. To be more specific, I was referring to the collocation advantage with life cycles. You don't have to switch between files.

[HriBB]

I'm actually hiding MUI behind a module

I have been doing the same on a large project, though on small ones I'm consuming it directly.

@oliviertassinari Would love to see an example of this, maybe like a recipe in the docs. I have just started using material-ui and was a bit sceptical about CSS in JS, but I like it.

[Izhaki]

@HriBB In my app, instead of

import Grid from 'material-ui/Grid'

I'll have

import {
    Grid
} from 'ui/controls'

And ui/controls.js will have:

export { default as Grid } from 'material-ui/Grid'

[oliviertassinari]

@HriBB Here is an example of a wrapper above the Paper as we are building a theme on top of Material-UI (doesn't look much like the material spec):

import React from 'react'
import classNames from 'classnames'
import PropTypes from 'prop-types'
import MuiPaper from 'material-ui/Paper'
import { withStyles, createStyleSheet } from 'material-ui/styles'

const styleSheet = theme => ({
  backgroundA100: {
    background: theme.palette.accent.A100,
  },
  backgroundA200: {
    background: theme.palette.accent.A200,
  },
  backgroundA400: {
    background: theme.palette.accent.A400,
  },
  padding: {
    padding: theme.spacing.unit,
  },
})

function Paper(props) {
  const { background, classes, className, padding, ...other } = props
  return (
    <MuiPaper
      elevation={0}
      square
      className={classNames(
        classes[`backgroundA${background}`],
        {
          [classes.padding]: padding,
        },
        className
      )}
      {...other}
    />
  )
}

Paper.propTypes = {
  background: PropTypes.oneOf(['100', '200', '400']),
  classes: PropTypes.object.isRequired,
  className: PropTypes.string,
  padding: PropTypes.bool,
}

Paper.defaultProps = {
  background: '100',
  padding: false,
}

export default withStyles(createStyleSheet(styleSheet))(Paper)

Also, as you can see, the plan with this issue is to be able to have the following diff on userland:

-export default withStyles(createStyleSheet(styleSheet))(Paper)
+export default withStyles(styleSheet)(Paper)

[oliviertassinari]

Options will be moved to the second argument:

export default withStyles(styleSheet, { name: 'Paper' })(Paper)

[Izhaki]

I've just added MUI v1 to another project.

I've refactored the create-react-app on a fork to show where we ended up after refactoring.

This is just to give some sense of a real use-case, albeit opinionated.

Note that currently the example excludes the concept of Document, which will change things slightly (eg, location of global JSS).

Highlights

Compared to the original example:

theme and stylesheet injection

We inject both theme and a stylesheet to our root, as we see these app-specific than the more general root.

components/Root.js:

const styleSheet = ( theme ) => ({
    '@global': {
        html: {
            background: theme.palette.background.default,
            WebkitFontSmoothing: 'antialiased', // Antialiasing.
            MozOsxFontSmoothing: 'grayscale', // Antialiasing.
        },
        body: {
            margin: 0,
        },
    },
})

const theme = {
    palette: createPalette({
        primary: purple,
        accent: green,
    }),
}

const Root = () => {
    return (
        <Router>
            <Route path="/" component={ App } />
        </Router>
    )
}

export default withUI( Root, theme, styleSheet )

injectSheet

We have used injectSheet.

components/App:

import styleSheet from './App.jss'

// ...

export default injectSheet( styleSheet, App )

components/App.jss:

export default
{
  container: {
    textAlign: 'center',
    paddingTop: 200,
  }
}

Hiding MUI

That's more for @HriBB - don't think an actual example should include this.

components/App:

import { Button } from '../ui/controls/';
import Dialog, {
  DialogTitle,
  DialogContent,
  DialogContentText,
  DialogActions,
} from '../ui/controls/Dialog';
import Typography from '../ui/Typography';

[oliviertassinari]

I have merged the first PR toward solving this issue. We should be able to remove createStyleSheet quite easily now.

[Izhaki]

Good job @oliviertassinari .

As an additional note, if you look at context.js and withRoot.js in the refactor, you'll probably recognise that these are rather generic and unlikely to change between applications (note that I don't think withRoot should be injected with styleSheet - something like Document should).

I do wonder if these can be merged into MUI, rather than having to let clients write them time and again?

Suffice to look at the repeation between the create-react-app example and the next one to see the generic patterns emerging.