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

CSS specificity #11

Open
rybon opened this issue Apr 25, 2016 · 17 comments
Open

CSS specificity #11

rybon opened this issue Apr 25, 2016 · 17 comments

Comments

@rybon
Copy link

rybon commented Apr 25, 2016

All generated classes have the same specificity, but that complicates extending generic styles with custom styles for a specific theme.

Example:

// Button.jsx

import React from 'react';
import classNames from 'classnames';

const Button = ({ className, children }) => (<button className={classNames(styles.button, className)}>{children}</button>);

const styles = cssInJS({
  button: {
    color: 'blue';
  }
});

export default Button;

// CustomButton.jsx

import React from 'react';
import Button from 'Button';

const CustomButton = () => (<Button className={styles.customButton}>Click me!</Button>);

const styles = cssInJS({
  customButton: {
    color: 'green'
  }
});

export default CustomButton;

When I render CustomButton, the color: 'blue' still takes precedence over color: 'green', but I want my green theming to override the default styling. Is there a mechanism to do that without resorting to specificity hacks?

@martinandert
Copy link
Owner

@rybon I found two issues with your Button functional component:

  • It think it should be (props) instead of ({ props })
  • It should be <button className=... instead of <button style=...

Maybe that already helps?

@rybon
Copy link
Author

rybon commented Apr 25, 2016

No, that was just some code I quickly wrote to illustrate the issue. I fixed the syntax. The issue remains though.

@martinandert
Copy link
Owner

@rybon I can reproduce that issue now. I'm working on fixing it but it'll take some time. I'll keep you updated.

@rybon
Copy link
Author

rybon commented Apr 27, 2016

Great! Thank you for picking this up. I appreciate all the work you put into this library.

My idea for a possible fix:

Say my root React component is <App className={styles.app} />. And I have <CustomButton className={styles.app.customButton} /> nested inside that component. Then the .app. would cause the generated CSS to be something like:
.src_components_App_js-styles-app .src_components_CustomButton_js-styles-customButton { ... }, which takes precedence over .src_components_Button_js-styles-button { ... }.
Then, I can create application specific styles to override the generic styles of the generic <Button /> component.
I'm not sure whether this approach is a good or bad idea though.

@jchristman
Copy link

jchristman commented Apr 27, 2016

I agree - Having a method of "netsting" classnames would allow for more specificity. As of right now, this generates an error, because "custombutton" is not one of the whitelisted pseudo elements that are allowed to be nested with objects:

const stylesheet = cssInJS({
    theme: {
        // Some styles here for general theming

        custombutton: {
            // styles for button here
            // these would theoretically be available with className={stylesheet.theme.custombutton}
            // and generate '.src_themes_js-stylesheet-theme .src_themes_js-stylesheet-theme-custombutton'
        }
    }
});

I think the main issue to figure out is the following: Is the object (that isn't a pseudo element) styling for specific subelements (i.e. .class div), or is it a nested class for specificity (i.e. .class .class2)?

I think the following syntax would make sense to distinguish between the two to allow for this functionality:

const stylesheet = cssInJS({
    theme: {
        // Some styles here for general theming

        // Treat all nested objects that aren't pseudo elements as element styling within the class
        // This generates the css rule .src_themes_js-stylesheet-theme div
        div: {

        },

        // Treat all nested objects preceded by a '.' as a nested class for specificity purposes
        // This generates the css rule '.src_themes_js-stylesheet-theme .src_themes_js-stylesheet-theme-custombutton'
        '.custombutton': {
            // styles for button here
            // these would theoretically be available with className={stylesheet.theme.custombutton}
            // and generate '.src_themes_js-stylesheet-theme .src_themes_js-stylesheet-theme-custombutton'
        }
    }
});

Thoughts?

@martinandert
Copy link
Owner

I think the problem is that the ordering of styles in the generated bundle CSS file isn't correct. See https://jsfiddle.net/11qmaeu4/ for an example on how order of styles matters (you are probably aware of that).

The idea I have to fix this is to construct a dependency graph of components while the babel transformation happens (maybe something like that is already built-in into babel). Then I'll do a topological sort of those dependencies and write the components stylesheets in the resulting order into the bundle css.

For example, given we have the following component dependencies:

A --+--> B --+--> C
    |        |
    |        +--> D
    |
    +--> C

Sorting that results in

A --> B --> C --> D

which will be bundled as

D's styles
C's styles
B's styles
A's styles

Maybe that helps? WDYT?

@jchristman
Copy link

I'm not quite sure that will work for all project structures. For example, assume the following project structure:

client/
    configs/
        themes/
            index.js   <-- This conglomerates all themes and allows importing them
            theme1.js   <-- This contains a cssInJS exported stylesheet
            theme2.js   <-- This contains another cssInJS exported stylesheet
    modules/
        core/
            A.jsx   <-- All of these components have their own cssInJS stylesheets
            B.jsx   <-- but also import classes from '/configs/themes'
            C.jsx
            D.jsx

In this case, how would you know where to stick the cssInJS generated classes into the bundle when the specificity of imported things might not be known? In other words, what if A, B, C, and D all depend on various classes from the imported stylesheet variables (from the themes directory)? How do those get ordered?

@rybon
Copy link
Author

rybon commented Apr 27, 2016

I tested this approach and got the styles to be generated in a reverse order, but that didn't seem to help as far as I could determine. I feel the solution is to provide an API that will make certain selectors more specific than others. In my case, nesting my theme classes under an 'app' class is enough. I suppose we'd have to walk the component tree starting from the root, analyse every cssInJS object for top-level key names pointing to a parent cssInJS object, and prepend that parent class in the resulting selector.

@martinandert
Copy link
Owner

martinandert commented Apr 28, 2016

What about introducing a special css rule that acts as a directive to prepend something to the generated classname? Example:

const PrimaryButton = () => (<Button className={styles.primary}>Click me!</Button>);

const styles = cssInJS({
  primary: {
    '@prepend': '.my-theme',
    fontSize: 24
  }
});

The above will be transformed to:

const PrimaryButton = () => (<Button className={styles.primary}>Click me!</Button>);

const styles = {
  primary: 'PrimaryButton_js-styles-primary'
};

And the generated CSS will be:

.my-theme .PrimaryButton_js-styles-primary {
  font-size: 24px;
}

@rybon
Copy link
Author

rybon commented Apr 28, 2016

Sure, that could work. The only thing that feels a bit off with that approach is that we deviate from standard CSS syntax. But fine as a fix for now I suppose.

@jchristman
Copy link

Is the @prepend syntax easier to support than allowing nested classes? What if this generated the same CSS as the @prepend syntax?

const styles = cssInJS({
  mytheme: {
    primary: {
      fontSize: 24
    }
  }
});

To me, this feels more intuitive and is more expressive in the code for use (i.e. className={stylesheet.mytheme.primary}) -- this suggests a prepended class. If I just access primary that has a @prepend, then the code is not expressive in suggesting that there is prepended classes (i.e. className={stylesheet.primary}).

@jchristman
Copy link

Oh I see what your main use case was. Is the idea that .my-theme is defined somewhere and then the @prepend would be defined somewhere else?

@steadicat
Copy link
Contributor

Here's a crazy idea that might solve all use cases. css-in-js could provide a special version of classnames that would build a precise “CSS dependency graph” between components and styles, and use that to sort CSS in the output.

E.g.:

const OuterComponent = () =>
  <InnerComponent className={classNames(outerStyles.redBorder)} />;

const InnerComponent = ({className}) =>
  <div className={classNames(className, innerStyles.blueBorder)} />;

While parsing this, when we encounter the first instance of classNames, we detect that OuterComponent applies outerStyles to InnerComponent. This builds a graph like this:

OuterComponent → outerStyles → InnerComponent

When we encounter the second classNames, the graph becomes:

OuterComponent → outerStyles → InnerComponent → innerStyles → div

Once parsing is done, this boils down to:

outerStyles → innerStyles

So we know to sort outerStyles after innerStyles.

I think an approach like this is a more general and more robust solution than having to manually increase the specificity of the selectors. Thoughts?

@steadicat
Copy link
Contributor

We could even take this one step further and consider the order of the styles inside the classNames call, and give precedence to the styles applied later. This would be very powerful, and allow control previously only possible with inline styles.

For example:

const InnerComponent = ({className}) =>
  <div className={classNames(innerStyles.defaultBorder, className, innerStyles.padding)} />

The intent here is that the border can be overridden, but the padding can’t. This can be achieved with a graph like this:

innerStyles.padding ↠ InnerComponent → innerStyles.defaultBorder → div
                             ↑
   OuterComponent → outerStyles.redBorder

That would be transformed into:

innerStyles.padding  → innerStyles.defaultBorder
                 ↓        ↑
           outerStyles.redBorder

Which results in:

innerStyles.defaultBorder
outerStyles.redBorder
innerStyles.padding

Implementation could be tricky, but it seems doable.

@rybon
Copy link
Author

rybon commented Aug 24, 2016

Great suggestions! Should be doable indeed. Maybe the PostCSS toolchain can help us out here.

@martinandert
Copy link
Owner

css-in-js could provide a special version of classnames that would build a precise “CSS dependency graph” between components and styles, and use that to sort CSS in the output.

Isn't that what the classnames package already does?

@steadicat
Copy link
Contributor

The JS API of the provided classnames would be the same, yes. But the existing classnames does nothing special to help the output of css-in-js styles. The proposed classnames would be special in that it would serve as a marker for the preprocessor (sort of like cssInJS()).

The preprocessor needs something to look for to detect when classes are applied to other components. I guess it could also just look for all occurrences of the className attribute, but I thought an explicit function call would make preprocessing easier.

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

4 participants