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

More control over merge ordering. #23

Closed
dchambers opened this issue Sep 15, 2016 · 6 comments
Closed

More control over merge ordering. #23

dchambers opened this issue Sep 15, 2016 · 6 comments

Comments

@dchambers
Copy link

dchambers commented Sep 15, 2016

First off, thanks for an awesome project that's helping to solve the CSS Modules theming conundrum 👍 .

Although @themr('injectedStyles', localStyles) allows the injected stylesheet to override the component-local styles (where the override granularity can be increased if that's helpful), there are other more complex ordering strategies that can be useful depending on the project, so I'd really prefer to have specific control over both the number of stylesheets being merged, and the order in which they are merged.

For example, in my case I'd really like to invoke as @themr('siteTheme', localStyles, 'componentOverrides'). My motivation for wanting to do this is that I'd like the ability to very quickly theme an app we have using a site-theme that involves the cross-cutting concerns that affect all components, or go further and start theming specific components when we can justify spending more time, while still retaining the ability to just use the app as it is, with its default theme specified directly within the React components.

I'm not suggesting here that 'react-css-themr' also supports the particular function signature I've used above, but that any number of theme objects (string | object) can be provided as suits the end-developer. It's possible that this might also address @gharwood1's issue?

@raveclassic
Copy link
Contributor

@dchambers If I understand you right you need an ability to mix multiple theme objects on component declaration. You could use themeable function to mix multiple themes in one and then pass it do decorator:

import {themr, themeable} from 'react-css-themr';
import css1 from './css1.css';
import css2 from './css2.css';
@themr('Component', themeable(css1, css2))
class Component extends React.Component {
 render() {
   const {theme} = this.props; //'theme' is mix of context, props, css1 and css2
 }
}

Please correct me if this's not what you need.

@dchambers
Copy link
Author

Hi @raveclassic, thanks for providing some options here. I wasn't aware of the themable function, so it's good to know about it.

While the presented solution allows more ordering flexibility around the local styles that the component itself provides, it doesn't add any flexibility around the themed styles that are being injected in from the outside -- 'Component' in your example -- whereas I really need some of my injected styles to be trumped by the local component styles, and others not to be.

@raveclassic
Copy link
Contributor

@dchambers You are welcome, themeable is just undocumented. =)
AFAIKS you need to force some of the base component styles (defined in decorator) to override the styles came from props/context, don't you? Well it's seems a bit unclear why would you do that.
Currently theme is merged with the following priority: context defined in ThemeProvider, base theme defined in themr and then component props and it seems reasonable. If you need to force component base styles to override what comes from props, then maybe you should consider using !importants or manually add these styles to props when rendering your component?

@dchambers
Copy link
Author

AFAIKS you need to force some of the base component styles (defined in decorator) to override the styles came from props/context, don't you?

We don't currently use the props approach because they don't help with the nested components that re-usable components will be using internally, but which also need to be themed. That leaves only the basic styling that the component provides and the context theming that the app provides, which isn't enough.

Well it's seems a bit unclear why would you do that.

The motivation for doing what I'm doing is that I'd like to create a set of re-usable components where each component provides some neutral styling so that it works out of the box, but where the components can be re-styled by each app that wants to make use of them.

The important difference is that I want there to be two approaches to app theming:

  1. Global theming: provide quick theming that affects all components, but allow re-usable components to override this with local styles where they know better, or think they know better.
  2. Component theming: allow individual component styling to be overridden to meet the theme requirements of the app where necessary.

As you point out, !important can be used, but this is quite awkward. Additionally, I discovered that I can also do what I want by applying multiple @themr decorators, as follows:

@themr('siteTheme')
@themr('noSuchTheme', localStyles)
@themr('componentOverrides')
class Component extends React.Component {
}

but this only worked if I also controlled the load order of the associated stylesheets, which is even more awkward.

Maybe I'm still misunderstanding this though. Although I've read the docs maybe ten times, I still struggle a bit to understand everything.

@raveclassic
Copy link
Contributor

We don't currently use the props approach because they don't help with the nested components that re-usable components will be using internally, but which also need to be themed. That leaves only the basic styling that the component provides and the context theming that the app provides, which isn't enough.

That's why I've opened #24

The motivation for doing what I'm doing is that I'd like to create a set of re-usable components where each component provides some neutral styling so that it works out of the box, but where the components can be re-styled by each app that wants to make use of them.

This is exactly what I'm also trying to achieve. I've ended up with not including base styles into components themselves but to include them at base context, here's an example. Then I can modify my library context in consuming project by just extending the default theme context. Additionally I can wrap plain components with additional styles that may be not included in context in consuming project.
Like:

//CustomComponent
import customCss from './Custom.css';
import Base, {BASE} from 'dx-components/src/components/Base/Base.jsx';
import baseCss from 'dx-components/src/components/Base/Base.css';
import {themr, themeable} from 'react-css-themr';
//one way is to attach styles here, note that base styles are not included at all
export default themr(BASE, customCss)(Base);
//the other way is to mix all styles
export default themr(BASE, themeable(baseCss, customCss)(Base);
//or just
export default themr(BASE, baseCss)(themr(customCss)(Base));
//or mix when defining new context
//context.js
import theme from 'dx-components/src/config/theme.js';
import {BASE} from 'dx-components/src/components/Base/Base.jsx';
//repeat all css-releated stuff here
export default {
 ...theme,
 [BASE]: themeable(baseCss, customCss) //any variations you prefer
}

by applying multiple @TheMr decorators

Yep, this is also undocumented =) Additionally you can use the same keys to avoid nested HOC wrapping.

@dchambers
Copy link
Author

That's why I've opened #24

Awesome 👍

This is exactly what I'm also trying to achieve.

This is perfect! I need to study your 'dx-components' library and try that approach out myself and see how I get on.

Like:

//CustomComponent
...

Thanks for this excellent code snippet showing all the different ways you can go about doing this. That really helps. It's also interesting to see that you're using the themr function instead of the @themr decorator (also undocumented 😄) as we're also using that approach as we use pure stateless components everywhere.

I'm going to close this issue until I've had a chance to try the approach you're taking. Thanks for all the help! 💯

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

No branches or pull requests

2 participants