javivelasco / react-css-themr

Easy theming and composition for CSS Modules.
MIT License
591 stars 68 forks source link

Deeply merging enhancement #19

Open gharwood1 opened 8 years ago

gharwood1 commented 8 years ago

Deeply merging: theme objects are merged and if a key is present in more than one object, the values for each objects are concatenated.

While this does offer the most flexibility, I think it can be improved to override styles set by parent themes/defaults.

<ThemeProvider theme={global}>
    <ThemeProvider theme={specialCase}>
        <App />
    </ThemeProvider>
</ThemeProvider>

Take this example. I think it would be cool to see specific nested styles defined in global overwritten if also defined by specialCase.

@javivelasco If I am understanding correctly, this might be pending work on postcss-apply?

Post Edit:

Note current work arounds are to use selectors that are more specific and/or override using !important.

javivelasco commented 8 years ago

I never thought about combining themes from an upper context. Right now the code assumes that you are using just one provider. Since usually providers are defined on the top of your app, maybe you can just merge both themes using Javascript? A deep merge from lodash should be enough.

Apart from that, maybe I'm lost in translation :/ How is this related with postcss-apply? You can always choose the way you compose your themes and if you want to override the full value of a class you can use softly compose:

Softly merging: theme objects are merged but if a key is present in more than one object, the final value corresponds to the theme with highest priority.

By priority I mean context < default < prop Sorry if I'm missunderstanding you!

gharwood1 commented 8 years ago

I'll try again via another example(ignore nested ThemeProviders) :

// Bundle Button with react-toolboxes built in theme.scss
import Button from 'react-toolbox/lib/button';

// import global overrides (written by me)
import { global } from '../../styles';

// Say I have a react container/component with a render method that uses ThemeProvider
render() {
    return (
      <ThemeProvider theme={global}>
          <Button />
      </ThemeProvider>
    );
  }

In this example I expect the default Button theme.scss(RTButton is the Key) to get merged with my overrides specified in global (also specified with RTButton).

An example attribute I would like to override could be:

Default

.button {
    position: relative;
}

Override by global

.button {
    position: fixed;
}

In this case they both get applied to the browser, but the Default takes priority. I definitely want to deeply merge, because I don't want to start from scratch with the react toolbox components.

Hope this makes more sense!

javivelasco commented 8 years ago

Oh I get it now. The thing is that we are getting modules but not the actual CSS. Processing the css is therefore out of the scope of this library. I know see what you mean with apply. It would work as something complementary but it has an important caveat (once this is resolved). Doing:

@import 'react-toolbox/lib/button/theme.css';

:root {
  --button-content: {
    color: red;
  }
}

And then importing that file would work like a charm... We could even expose apply for every node same as we do with classes. But you are importing the whole theme.css so your styles will be duplicated :(

I don't see a way of achieving this right now if it's not with what some people call "functional styles". That means rewriting the styles thought...

gharwood1 commented 8 years ago

@javivelasco No worries, Like I mentioned. One Work around is to include the !important flag. Thats usually a big no no, but In this case since we are keeping things modularized, I think I am okay with it. I'll keep an eye out on the postcss apply issue/enhancement.

javivelasco commented 8 years ago

In any case it's better if you check the priority of the selector in the browser and boost your own by replicating the same plus a classname for example. I have to revisit react toolbox styles to try to reduce specific selectors as much as possible.

Anyway apply will solve some issues soon :)

dalinarkholin commented 7 years ago

@javivelasco any news here? I believe https://github.com/pascalduez/postcss-apply/issues/10 has been closed. I haven't had time to test.

javivelasco commented 7 years ago

Not actually @gharwood. Honestly I think we can do nothing with ordering. This issue should be solved by controlling how modules are bundled together from your bundler config and overriding defaults with postcss plugins. Otherwise boosting priority is the only solution I think

petemill commented 7 years ago

@javivelasco can we supply an example of how to override defaults with postcss plugins?

javivelasco commented 7 years ago

Of course @petemill

mpodlasin commented 7 years ago

I would actually be super interested in merging themes in ThemeProvider if there was already ThemeProvider higher in component tree. We have a multi-module application where you would define styles in app root (first ThemeProvider) and then modify them on per module basis.

Would you be ok with such change? I am willing to make a pr, if needed.

raveclassic commented 7 years ago

@Podlas29 Actually I also need such feature but just do not have time to make a PR :)