jss-theme-reactor icon indicating copy to clipboard operation
jss-theme-reactor copied to clipboard

Sheet with other sheet as dependency?

Open Alxandr opened this issue 8 years ago • 8 comments

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).

Alxandr avatar May 01 '17 16:05 Alxandr

@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 avatar May 02 '17 12:05 oliviertassinari

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

oliviertassinari avatar May 02 '17 12:05 oliviertassinari

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.

oliviertassinari avatar May 02 '17 12:05 oliviertassinari

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.

Alxandr avatar May 02 '17 12:05 Alxandr

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.

oliviertassinari avatar May 02 '17 13:05 oliviertassinari

@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 <Page>`) 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' }.

Alxandr avatar May 02 '17 13:05 Alxandr

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.

oliviertassinari avatar May 02 '17 16:05 oliviertassinari

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.

Alxandr avatar May 02 '17 17:05 Alxandr