nathanmarks / jss-theme-reactor

NOT MAINTAINED Powerful theming layer for use with the jss CSS in JS library
MIT License
64 stars 6 forks source link

Sheet with other sheet as dependency? #42

Open Alxandr opened 7 years ago

Alxandr commented 7 years ago

I would like to create a stylesheet which overrides how something else looks within it's hierarchy.

Something like this maybe?

// button.js
export const styleSheet = createStyleSheet('Button', () => ({
  button: {
    color: 'black'
  }
}));

@withStyleSheet(styleSheet) // I made this decorator
export class Button {
  render() {
    const { classes, ...props } = this.props;
    return <button className={ classes.button } { ...props } />;
  }
}
// page.js
import { Button, styleSheet as buttonStyleSheet } from './button';

export const styleSheet = createStyleSheet('Button', [ buttonStyleSheet ], (theme, buttonStyles) => ({
  page: {
    color: 'black',

    `& ${buttonStyles.button}`: {
      color: 'pink' // buttons in pages are pink
    }
  }
}));

@withStyleSheet(styleSheet) // I made this decorator
export class Page {
  render() {
    const { classes, ...props } = this.props;
    return <div className={ classes.page } { ...props } />;
  }
}

When you're only working with your own styles this is probably not necessary, however I'm currently trying to override some styles from material-ui, which results in the following mess:

const styleSheet = createStyleSheet('RootAppBar', () => ({
  tabs: {
    height: '100%',

    '& > div': {
      height: '100%',

      '& > div': {
        height: '100%',

        '& > div:first-child': {
          height: '100%',
          alignItems: 'center',
        },
      },
    },
  },
}));

It would be a lot better if I could target the divs here by name (especially the :first-child one).

oliviertassinari commented 7 years ago
@withStyleSheet(styleSheet) // I made this decorator

You can use the withStyles as inspiration.

It would be a lot better if I could target the divs here by name

I would rather use the opposite pattern, creating classes with one level of specificity and applying the classes on the right elements. That's the pattern we encourage, we have made API and implementation tradeoff with the use-case in mind. That's a quick a common design choice share with the other CSS-in-JS library available.

oliviertassinari commented 7 years ago

The styled-jss API goes even one step further by making the class name concept an implementation detail.

oliviertassinari commented 7 years ago

We only expose buttonStyleSheet for test purposed. I have never seen your pattern before. It funny how people can tweak things around 😄 . Would my above propose solve the issue? Also, we don't want Material-UI users to be coupled with our styling solution. You don't have to use JSS. You can use whatever other solution that best fits your needs. Still, there is a styling infrastructure available for simplicity.

Alxandr commented 7 years ago

I'm not quite sure what your proposed solution is (I'm probably reading this wrong). Specifically what I'm trying to do is put Tabs into an AppBar. Problem is that Tabs output a bunch of stuff which does not accept xxClassName type properties, so to override it I have to either know the generated classnames, or I have to use the hierarchy (like I've done above). At least that's the only two solutions I can come up with. And while it can be said that in this case we just need to add xxxClassName to Tabs (etc.), there is likely to always be cases where there is something you can't restyle in Material-UI (or any other library that's built on jss-theme-reactor). Having the ability for style sheets to depend on other style sheets would alleviate that (at least to some extent).

The thing to note here is that I'm not in any way getting the class names from button outside of a render stylesheet context. Therefore the class names is still an implementation detail, I just accept them as a variable to my style generating closure, just like the theme variable.

oliviertassinari commented 7 years ago

And while it can be said that in this case we just need to add xxxClassName to Tabs (etc.)

That's something we want to do!

@Alxandr I better understand your use case, thanks.

there is likely to always be cases where there is something you can't restyle in Material-UI

What about users not using JSS? They would be trapped. I'm wondering it that shouldn't be solved at another level.

Alxandr commented 7 years ago

@oliviertassinari In my mind that point is somewhat moot. Sure, when issues arrive, you add xxClassName (or classes.xx or whatever you end up deciding is the best path), but for better or worse, jss is a dependency of material-ui. So for people who want to do really weird stuff (or simply want to override how a button looks if and only if it's contained within a `) I think it would be fine to say at that point you need to crack out your JSS.

Alternatively, you could split jss-theme-reactor (or the way material-ui does things) in two, so that instead of an API that goes (theme, [ ... dependent sheets ]) => style object, you'd do (theme, [ ... dependent sheets ]) => classes object. That way people could take the theme object and the classes object from the dependent sheets and do whatever with them. They could use JSS, or any other way to generate { page: 'my-magical-page-class' }.

oliviertassinari commented 7 years ago

or classes.xx or whatever you end up deciding is the best path

Yes, that's why I think it's the best path going forward as it would by default exposes all the style customization point you will ever need. I have seen so many PRs on the master branch to add xxClassName. That was inefficient.

or simply want to override how a button looks if and only if it's contained within a

Definitely not a weird use case :p. Another level to answer that issue is to write CSS in a way we only need to override the root. For instance, by using color inheritance or pudding over fixed height.

I'm prioritizing the doc section around the style override.

I'm not sure to understand your proposition. That sounds like exposing a classes property then feeding it to JSS or a helper to merge the class names.

Alxandr commented 7 years ago

I'm not talking about a property at all. I'm talking jss-theme-reactor (no react involved in that part).

Currently the API to create a stylesheet is createStyleSheet(name: string, fn: Theme => JssObject): StyleSheet. My first proposal was to extend this api to: createStyleSheet(name: string, deps?: Array<StyleSheet>, fn: (Theme, Array<ClassesMap>) => JssObject): StyleSheet), and last I said one alternative might be to split it into two.

Imagine something like this:

import { createStyleMap, createJssStyleMap } from 'jss-theme-reactor';

// we have a theme configured that has dark: true.

// classes gotten from bootstrap or whereever, could be css modules or what have you.
const buttonStyles = createStyleMap('Button', (theme) => {
  if (theme.dark) {
    return { root: 'btn-dark' };
  } else {
    return { root: 'btn' };
  }
});

// this uses JSS to generate and insert stylesheet
const muiButtonStyles = createJssStyleMap('MuiButton', (theme) => ({
  root: {
    margin: 5
  }
});

const pageStyles = createJssStyleMap('Page', [buttonStyles, muiButtonStyles], (theme, [button, muiButton]) => {
  // button = { root: 'btn-dark' };
  // muiButton = { root: 'Jss-generated-css-class-name' }
  return {
    root: {
      `& ${button.root}`: {
        color: 'red'
      }
    }
  };
});

Calling render on the result from createStyleMap is essentially a no-op. It only need to get the classes object. And it is not dependent on jss in any way.