Khan / aphrodite

Framework-agnostic CSS-in-JS with support for server-side rendering, browser prefixing, and minimum CSS generation
5.35k stars 188 forks source link

allow css() to take a string (normal css class name) & add it to the className #174

Closed jaredly closed 6 years ago

jaredly commented 7 years ago

so that you can do css(styles.something, "MyLegacyClassName") instead of css(styles.something) + " MyLegacyClassName".

It's a little thing, but I'd sure like it.

ethangodt commented 7 years ago

Have been using classnames for this. Would be great to just do it with aphrodite.

jaredly commented 7 years ago

another common case: css(styles.something, this.props.className)

xymostech commented 7 years ago

I feel like we've discussed this before, but I can't find the conversation. Overall though, I'm against this change.

For one, the point of having a css() function (and not just being able to use classNames with Aphrodite-generated class names) is to ensure that later styles override earlier styles. Since we have no control over the styles in the class name, we wouldn't be able to ensure this for the class names passed in, which would be sad.

Compounding on that, JavaScript is dynamically typed, so I can no longer tell by your css() call what is going to happen, which seems annoying to me. It might generate styles and append a class name, but someone could try passing aphrodite styles to it which would be different.

I'm happy to hear arguments for, but making Aphrodite do more to save a tiny bit of typing doesn't seem worth it to me.

jaredly commented 7 years ago

I actually see the second point as a plus :D "what's going to happen" is "aphrodite will do the appropriate thing". It's unfortunate that there's no way to enforce precedence when using legacy CSS class names, but I don't think that's a deal breaker. the css() function already handles nulls nicely, and so being able to do css(styles.something, this.props.isActive && "Legacy_active") or something would also be great.

It's fine if you don't want to add it, but I feel like it will make gradual adoption of aphrodite more appealing.

tkh44 commented 7 years ago

@xymostech I tend to agree with not having the strings appended, but the argument that you won't know what will happen is not a very solid one. Because of !important on the styles (in default mode) you know that your aphrodite generated styles will take precedence.

We might as well list some pro's for this feature

wellguimaraes commented 7 years ago

For now, I'm using a helper fn (workaround) as a replacement of css fn. It's basically a wrapper of css and classnames to achieve this feature.

import { css } from 'aphrodite';
import classNames from 'classnames';

export default function() {
  const styles  = [];
  const classes = [];

  [].forEach.call(arguments, it => {
    if (it && it._definition && it._name)
      styles.push(it);
    else
      classes.push(it);
  });

  return classNames.apply(null, [ ...classes, css(styles) ]);
}
lencioni commented 7 years ago

This was discussed in #8

winterbe commented 7 years ago

I like the proposal of @jaredly because it'd make the integration of Aphrodite into existing projects much easier without the need for helper functions like @wellguimaraes described.

lencioni commented 7 years ago

Considering how easy it is to achieve your desired result by using a wrapping function, I don't see any compelling reason to expand Aphrodite's API to support this use-case.

lencioni commented 6 years ago

This issue seems stale, and the maintainers who have commented seem to be against this proposal, so I'm closing it out. Thanks for the suggestion @jaredly!

gabssnake commented 6 years ago

Both mentioned use cases are quite common for us, what is the "proper" way to handle them?

// Modify styling via props?
css(styles.something, this.props.className)
// Integrate with legacy/external CSS code?
css(styles.something, "MyLegacyClassName")
tremarkley commented 5 years ago

you could also just className={`${css(styles.something)} ${this.props.className}`}

emin93 commented 5 years ago

Unfortunately this doesn't do false-checking, so you'll end up having undefined, false, etc. class names. <MyComponent className={isActive && 'my-active-class'} /> would render false as a class name which you probably don't want. I tend towards using the classnames library as long as the maintainers are against including it in the css function.