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

classList wanted #309

Closed
piranha opened this issue Sep 2, 2013 · 8 comments
Closed

classList wanted #309

piranha opened this issue Sep 2, 2013 · 8 comments

Comments

@piranha
Copy link
Contributor

piranha commented Sep 2, 2013

Adding a classList property on included components synchronised with domNode.classList would be nice to have. Now that I think about this it's the same as <div class={' '.join(classList)}>, but it will look much cleaner, don't you think?

@andreypopp
Copy link
Contributor

+1

As another option, we could just accept array as a value for class/className prop (not sure which variant is picked after all). This would be in line with style prop which accepts values of type Object.

@petehunt
Copy link
Contributor

petehunt commented Sep 2, 2013

At FB we use this function https://github.com/facebook/react/blob/master/src/vendor/stubs/cx.js

It can be used like this:

<div class={cx({myElement: true, isActive: this.state.active})}>...</div>

We've found that this is a much more declarative and succinct way to build up a CSS class list. GIve it a try and see how you like it. We've been chatting on and off about baking this functionality into React somehow but we hate the cx name (what does that even mean!?) and aren't sure where to put useful utility functions yet. If you have any ideas or advice here we'd love to hear it.

@andreypopp
Copy link
Contributor

@petehunt thanks, that's exactly what I need. Though it still can be cool if class prop would be built using this function automatically, like style prop.

@petehunt
Copy link
Contributor

petehunt commented Sep 2, 2013

Pre-open source we had a classSet prop on everything that did this but it was nixed. @tomocchino @yungsters may know why we did it -- I think we just wanted to simplify things.

@piranha
Copy link
Contributor Author

piranha commented Sep 2, 2013

I agree with @andreypopp, class could accept either a string or an object in that format, that would be useful. Just for the reference, Angular's ng-class does exactly that.

@jordwalke
Copy link
Contributor

classSet is awesome, and we still use it today internally in the form of the cx utility helper. It helps you declaratively specify the set of classes in a really readable form.

var classes = cx({
     Button: true,
     BigButton: !this.state.userIsPressing,
     RedButton: this.state.accountIsOverdue
});

We also have an internal transform step, that turns this into a fast string concatenation, without spending an object allocation: something like:

var classes = 'Button' + !this.state.userIsPressing ? 'BigButton` : '' + this.state.accountIsOverdue ? 'RedButton' : '';

The elegance in that transform is that it is not required - the runtime version of cx returns the exact same result, only not as fast/efficient as cx.

@sophiebits
Copy link
Collaborator

classSet is now available as React.addons.classSet (not inlined at transform time, but is otherwise good).

@zpao
Copy link
Member

zpao commented Dec 20, 2013

I'm going to say we should stay away from this for now and just use helpers (like the one @spicyj mentioned now exists)

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

6 participants