What's the easiest/cleanest way to work with class names?
Closed this issue · 9 comments
From reading the docs, it seems like Glamor prefers not using class names, and instead pushes people to use <div css={ ... } or <div ...css({ ... }). As far I can you can get a class name by forcing the resulting object .toString(), but that leaves me doing things like:
let classes = `${className || ''} ${STYLES.separator}`
if (bottomMargin) classes += ` ${STYLES.bottomMargin}`
if (topMargin) classes += ` ${STYLES.topMargin}`Previously with CSS Modules I was using the classnames package, but I think even to make that work I'd still need to do some ugly manual toString-ing, like:
const classes = classnames(className, `${STYLES.separator}`,
[`${STYLES.bottomMargin}`]: bottomMargin,
[`${STYLES.topMargin}`]: topMargin,
})Am I missing something? Is there any nicer way? It would make using Glamor awesome if so.
As a side thought about interoperability...
The focus on the more "magic" syntaxes (eg. the data-object-spreading approach or the custom-react-prop approach) seems unfortunate to me since standardizing around class name strings or inline style objects seems like the way to guarantee interoperability with the greater ecosystem of React (or non-React) components, since they are never going to assume a Glamor dependency (nor would you want them to).
I saw that css={ ... } to className="..." conversion was added in #95 but even that feels like too much magic in my opinion, since no one is going to know that from just looking at it.
Totally possible that I could be missing something that the current approach offers, if so would love to learn it!
Is this what you're looking for? (using computed property keys)
const rule1 = css({
boxSizing: 'border-box',
borderWidth: 0,
borderStyle: 'solid',
margin: 0,
padding: 0,
font: 'inherit',
textTransform: 'inherit',
textDecoration: 'none'
});
const classes = classNames({
[rule1]: true,
[css({fontSize: 50,marginRight: 5})]: true
});
console.log('classes', classes);
// css-lbcvys css-11iolpsHey @ChristopherBiscardi thanks for the example. That is essentially what I'd like to be possible, yup! Unfortunately, as far as I can tell, because of the way that Glamor defaults to returning an object that can be spread across a low-level JSX node, such as...
{
'data-css-1l8ukt1': ''
}...that example you provided actually doesn't work. Instead you'd end up with:
console.log('classes', classes)
// ""And to fix it you would have manually force the returned object to a string, like...
const classes = classNames({
[`${rule1}`]: true,
[`${css({fontSize: 50,marginRight: 5})}`]: true
});
console.log('classes', classes);
// css-lbcvys css-11iolps...which I think is super unfortunate, purely because the "default" case is optimized for a non-standard way of creating/sharing/defining styles.
If Glamor were to return class name strings by default, you could do simple things like:
const classes = classNames({
rule1,
css({fontSize: 50,marginRight: 5}),
props.isBold && css({ fontWeight: 'bold' }),
});that example you provided actually doesn't work
That's strange since I copied it out of a working codepen. Can you provide more details as to why it isn't working for you?
Ah yup, you're right, sorry, with [computed]: true it works because it coerces them. What I tested that didn't work was the nicer, terser version:
const glamor = require('glamor')
const classnames = require('classnames')
const styles = {
button: glamor.css({
background: 'gray'
}),
active: glamor.css({
background: 'red'
}),
}
const isActive = true
const classes = classnames(
styles.button,
isActive && styles.active
)
console.log('classes', classes)
// ""Perhaps to be more clear, toString is defined on styles returned from css(), which the object initializer spec says toString is called on if a computed property key is an object. Thus it's like calling css({}).toString()), which yields the css classname.
styles.button in your example will be an object and is not a computed property so it doesn't get toStringd. The approach you're taking looks a lot like the aphrodite glamor package. Maybe you want that?
Thanks! Yeah I understand about .toString() needing to be called—although I didn't realize that it was implicitly called on computed properties, although that makes sense.
What I'm talking about at a deeper level is arguing that from a architecture/design point of view, Glamor should put a stronger emphasis on class names (and/or potentially style dictionaries, although they are slightly harder to work with and will probably be less performant) because they are the portability "standard" of styling components. Right now there are a few different places that you end up having to be aware of the not-quite-classname-like behavior of Glamor when working with other libraries:
- When using something like
classnames. - When passing styles into a third-party component you have to be sure to stringify first.
- I'm sure there are more...
Which is the cause of not dealing in a common primitive.
Or put more directly:
What are the benefits from the current
data-*design choice? And are they significant enough that they are worth extra code and mental overhead for common use cases for users, in addition to extra bloat in the generated output to account for bothdata-*and class names, since class names are always required when crossing library boundaries? I'm my opinion they aren't, but I might not have the full picture.
I'm especially interested in arguing for this because I think Glamor seems like it has a lot of potential in terms of its simplicity, and with the css`...` syntax coming up.
Like you mentioned, I could use the Aphrodite-mimicking export from Glamor, but I'd personally argue for all of the X-like exports to be removed because they only serve to clutter the core library and make it feel more like a random experimental project without direction. I wouldn't want to depend on them heavily.
First, the original question -
let classes = `${className || ''} ${STYLES.separator}`
if (bottomMargin) classes += ` ${STYLES.bottomMargin}`
if (topMargin) classes += ` ${STYLES.topMargin}`would be simpler as
let classes = css(
rule,
STYLES.separator,
bottomMargin && STYLES.bottomMargin,
topMargin && STYLES.topMargin
)assuming all of these are glamor rules, of course.
on a broader note, I don't actually agree that classnames should be what the community standardizes. I wrote a bunch of words on this https://github.com/threepointone/glamor/blob/master/docs/css-as-values.md. The idea of having a rule as an opaque object is fairly core to the idea, letting me do stuff like WeakMap caches, composition, etc. the classnames issue can be fixed by aliasing it with a version of the function which recognizes glamor rules (and the computed keys syntax works well in a pinch).
Personally, I don't really feel the pain of non-readable classnames anymore. Unlike regular css (and I feel this is key to the issue), it's trivial to know where the definition of a rule happened by just looking at its 'callsite'. I do plan on making a debug helper to read current values from rules (I currently use cssFor(), but would prefer an object to inspect instead)
I was trying to use class-names to combine styles from a public class-based style-override API (parents may not want to use glamor) and ran into this.
For that case, one option is to just avoid the problem -- instead of combining class names, use the spread syntax for your glamor styles, and classNames for the parent overrides.
const myStyles = css({ color: 'red' });
const MyComponent = ({ className }) =>
<div {...myStyles} className={className}>...</div>;There may be specificity gotchas with this approach since class and attribute selectors have equal precedence, but it's working for us so far.
nothing actionable here, closing, feel free to reopen if there's more to discuss.