Default styles for components are difficult

[zgotsch]

When I have a component which wraps a component with the intention of putting some default styles on it, e.g.

class FooterLink extends React.Component {
  render() {
    let {className, ...props} = this.props;
    className = [className, css(styles.footerLink)].join(' '); // how to merge?
    return <a className={className} {...props} />;
}

it's not obvious how to merge the class names. If you simply use both of the generated class names (as in the code above), there is no way to enforce precedence (later injected style wins, presumably). Of course you could allow an array of style objects to be passed in and then css them all together, but this changes the API so the component isn't really a pure wrapper anymore.

Any thoughts on this?

[jlfwong]

@xymostech probably knows better, but I think we've been doing the "css them all together" approach.

I'm not quite sure what you mean by making it not a pure wrapper any more.

Edit: I re-read your example and understand now -- it's awkward in the case that the sole purpose of the component is to wrap another element with some special styles, so you want the props to lineup 1:1 with the underlying component.

Hmm.

One option is to make css remember not only which classnames it's generated in the past, but remember the style definition objects it merged to make them. Then could recombine them for you.

Your component would then just work like so:

class FooterLink extends React.Component {
  render() {
    let {className, ...props} = this.props;
    return <a className={css(className, styles.footerLink)} {...props} />;
}

With FooterLink still being invoked like so:

<FooterLink className={css(otherStyles.customLink)} href="/tothepast" />

So the inner className expands to css(css(otherStyles.customLink), styles.footerLink).

The API change would be to make css() understand string arguments that match styles it previously generated.

[zgotsch]

That API makes sense to me. You could also prefix created class names with something and identify generated ones based on that.

[montemishkin]

Perhaps I'm missing something here, but it seems like the only modification to css needed is that it join any string arguments passed to it onto the resulting class name (separated by spaces).

Something like this (based on the current implementation of css):

const css = (...args) => {
    // Filter out falsy values from the input, to allow for
    // `css(a, test && c)`
    const validDefinitions = args.filter((def) => def && typeof def !== 'string');
    // interpret all string arguments as css class names
    const classNames = args.filter((s) => typeof s === 'string');

    // Break if there aren't any valid styles or class names
    if (validDefinitions.length === 0 && classNames.length === 0) {
        return "";
    }

    const generatedClassName = validDefinitions.map(s => s._name).join("-o_O-");
    // only need to inject the generated class since string arguments will have already been
    // injected by the `css` calls that generated them
    injectStyleOnce(generatedClassName, `.${generatedClassName}`,
        validDefinitions.map(d => d._definition));

    // join generated class name and passed class names
    const className = [...classNames, generatedClassName].join(' ');

    return className;
};

For example, if css(styles.fancy) returned "fancy_j239sk" then css("hoverable", styles.fancy) would return "hoverable fancy_j239sk".

Then you could implement FooterLink just like @jlfwong suggested:

class FooterLink extends React.Component {
  render() {
    let {className, ...props} = this.props;
    return <a className={css(className, styles.footerLink)} {...props} />;
}

and use it (again like @jlfwong suggested) as:

<FooterLink className={css(otherStyles.customLink)} href="/tothepast" />

(:+1: on the link to the past btw!)

I'd be happy to make a PR along these lines if this seems like the right approach. Am I missing something obvious here?

[jlfwong]

@montemishkin The problem is precedence. The reason why css(styles.a, styles.b) generates a_abc123-o_O-b_def321 instead of just a_abc123 b_def321 is to ensure that we retain the correct precedence order.

Otherwise, if we have:

.a_abc123 { color: red; }
.b_def321 { color: blue; }

we get a different result from

.b_def321 { color: blue; }
.a_abc123 { color: red; }

So instead we control precedence manually in JavaScript by merging the style objects, and produce:

.a_abc123-o_O-b_def321 { color: blue; }

Notice that the result is "blue" because b was specified afterwards. It's important to note that css(styles.a, styles.b) isn't necessarily equivalent to css(styles.b, styles.a).

If you have css(styles.a, "other-class"), and we were to generate a_abc123 other-class as the class name, if the two rules affected similar styles, you would get non-deterministic results depending on whether styles.a was injected into the page before or after "other-class".

[montemishkin]

Ah, yes I understand now. Thanks for taking the time to explain!

In that case, @jlfwong's idea:

One option is to make css remember not only which classnames it's generated in the past, but remember the style definition objects it merged to make them. Then could recombine them for you.

seems like a good solution.

I'll try to see if I can come up with any other ways to solve this, but in the mean time I'd be happy to take a stab at implementing the suggested approach. Does anybody see any reasons to not go for that approach?

[montemishkin]

One issue will be with server side rendering. Once we are on the client, there will be no way of determining which JS objects generated the renderedClassNames at the call to StyleSheet.rehydrate(renderedClassNames).

[jlfwong]

One issue will be with server side rendering. Once we are on the client, there will be no way of determining which JS objects generated the renderedClassNames at the call to StyleSheet.rehydrate(renderedClassNames).

This is a good point and an important one to address. We could pass along that information to the client-side, but that idea doesn't greatly appeal to me as it will dramatically increase the size of the HTML payload we sent to the client just for this purpose. I don't have any great ideas to address it off the top of my head.

[montemishkin]

Agreed!

Another issue: what to do when a string passed to css is a CSS class name that was not generated by aphrodite?

For example, somebody using an external CSS library that had some class .fancy might want to do something like

css(stylesObject1, "fancy", stylesObject2)

Well, maybe nobody would really want to do that, but it might easily arise in a situation like that described in the original issue post.

One (totally crazy) idea that would potentially solve this concern, as well as that of server side rendering, would be if we could convert CSS back into a JS object. Since the external stylesheet could be on the local machine or on a CDN, we would potentially have to fetch some of the resources. The api might look like

StyleSheet.registerCSS('./path/to/my/styles.css', 'http://some.url.com/path/to/styles.css')

(the user would then not load these stylesheets in their HTML)

StyleSheet.registerCSS (or some better name) would then turn each css class in the given resources into a JS object and remember which objects correspond to which class names. Thus calls to css would know what to do with class names found in these stylesheets.

Yes, I am aware that is crazy. But its the only idea I've got at the moment, and at the least maybe it will help us along the path to a better solution.

[xymostech]

Fetching and parsing external CSS doesn't sound like a very robust system to me. I feel like most selectors in CSS files are nested selectors (i.e. .a .b > .c) whereas Aphrodite only supports single classnames with a sprinkling of pseudo-classes and media queries. So most of the styles in external CSS files wouldn't be able to be imported by Aphrodite anyways.

[montemishkin]

I completely agree. Just figured I'd throw something out there to try and get the ball rolling towards a more realistic solution :)

[montemishkin]

This pattern of merging default styles with styles passed in as props is common in my projects. In most cases, I have access to the JS style objects, and so I would be able to avoid this problem of merging CSS classes by doing something along the lines of

const OuterComponent = () => (
    <div className={css(outerStyles.red)}>
        <InnerComponent style={outerStyles.blue} />
    </div>
)

const InnerComponent = ({style}) => (
    <span className={css(innerStyles.green, style)}>
        hi
    </span>
)

However, this is an unfortunate workaround. For one, it still does not solve the problem of being able to transparently pass classNames to wrapper components (whether the className is generated by aphrodite or comes from an external CSS library). But beyond this, it suffers numerous other downsides, the most notable of which is:

InnerComponent must now expect an aphrodite style object for its style prop, rather than a JS style object. Thus it cannot be packaged up and distributed without the user needing to also carry around a call to aphrodite's Stylesheet.create. A way around this would be to instead do

const InnerComponent = ({style}) => {
    const createdStyle = StyleSheet.create({style}).style

    return (
        <span className={css(innerStyles.green, createdStyle)}
            hi
        </span>
    )
}

But then I can no longer pass an aphrodite style object down from OuterComponent, and I will instead have to have two stylesheets in my file for OuterComponent (one which went through StyleSheet.create and will be immediately given to css, and another which is just a JS styles object and will be passed as a prop to InnerComponent).

[montemishkin]

I'm wondering if anybody has alternate patterns that might altogether sidestep this issue? What does the Khan Academy team do (or not do) about this?

[jlfwong]

This is an incomplete answer to the issue, but w.r.t to the "Thus it cannot be packaged up and distributed" issue, you can allow users of your component to accept regular inline styles if you'd like to avoid an external API dependency on Aphrodite.

If you do wish to allow users to style via a classname, you can do <span className={css(innerStyles.green) + " " + passedInClassName} />, you just get no guarantees of the ability to override the given styles. In cases where you want the user to specify styles, hopefully you aren't needing to specify a lot of styles that would need overriding anyway.