Added support for collections in :class property

[zoldar]

That's my first stab at adding support for passing a collection for class property, as mentioned in #138. Is that the right place to implement it?

[whodidthis]

I think it could be useful to also add support for conditional classes, kind of like what react devs had in mind for inline styles https://github.com/reactjs/react-future/blob/master/04%20-%20Layout/04%20-%20Inline%20Styles.md#using-styles.

So remove falsey values to enable ["my-input" (and @active "active")].

Also wouldn't mind to have those conditional inline styles implemented heh

[zoldar]

@whodidthis Good idea. Added filtering step.

[GetContented]

Why filter identity here? isn't this a no-op, or at best the equiv of seq?

[GetContented]

oh, ok. Nevermind. I understand now - I read the comments.

[GetContented]

You can already do this with clojure's built-in functionality. This might be best suited by writing a helper method.

Adding this code adds at best an additional if statement around every single class in our apps. What if we don't want this in our code? At worst, the filter adds quite a bit of code to these collections, and ends in some magic. At best, another conditional.

The more magic we have in our code, the more bloated and less understandable it becomes. One would think this comment isn't necessarily relevant for this patch, but it's actually the little things that matter.

Clojure, and Reagent are awesome precisely because they are tiny, comprehensive and understandable. Precisely because of the magic they don't have.

[zoldar]

@JulianLeviston I agree that adding feature increasing code complexity just because it may prove useful is not a good idea. But in this actual case, maybe the added complexity is balanced out by how often it's found useful? It would be probably best if others had their say on this before deciding what to do about the PR.

[GetContented]

@zoldar Sure. It's only my two cents, afterall :)

[Deraen]

(filter identity) is unnecessary as nil is anyway outputted as empty string, it might add some unnecessary spaces but that shouldn't be a problem.
Also instead of interpose and apply str, clojure.string/join should work.

(defn stringify-class [{:keys [class] :as props}]
  (if (coll? class)
    (assoc props :class (string/join " " class))
    props))

[zoldar]

@Deraen Good points. Also, string/join actually does omit putting separator after nil values - neat.

[whodidthis]

false is printed as "false" though, or is there some other more obvious way than
(and @my-atom "my-class")

[Deraen]

(if @my-atom "my-class") but I guess it would be best if and worked also.

[whodidthis]

Right, when and if. I guess i translated && a bit too directly heh.

Although i guess i would still go with filtering out falses, (and false 1) returns false and may trick people although maybe not too many use a class called false.

Also using and would still make sense in (and @dirty @warning "warning") etc in my opinion

[zoldar]

@whodidthis @Deraen I also think that it will be best and least surprising if all falsey values are removed. Brought back the filtering by identity step.

[mike-thompson-day8]

I'm in favour of merging this.

Update
But I'm wondering how/where it should be documented. It is all very well for us to know of this feature, but what about the great unwashed :-)

[holmsand]

Yes, I agree: it might be convenient to have. Let's do this after 0.5.1. (There should probably be some tests for this, though...)

@mike-thompson-day8 Yes, we should definitely have some visible documentation of especially syntax. Ideally that should be on the website – as that is most visible, I guess?

[zoldar]

Added tests for the feature.

[zoldar]

Finally got my s**t together and learned how to use Git properly (hopefully). Now everything is squahsed into a single, nice commit.

[runningskull]

👍

[bsteuber]

+1

[zefirka]

+1

[danielcompton]

Should this be sequential? instead of coll?. Maps and sets also implement ICollection.

[zoldar]

I have used coll? in order to support sets actually (which is not covered by tests as I see it now, bummer). Do you think it's impractical/unnecessary?

[bsteuber]

Supporting sets is good, so just leave it that way.
I guess the remark meant to ask how to deal with maps. Maybe throwing an
error would be best, but probably "undefined behavior" is okay too - I
mean, who would do that anyways :-P

Adrian Gruntkowski [email protected] schrieb am Mi., 28. Okt. 2015
10:19:

In src/reagent/impl/template.cljs
#154 (comment)
:

@@ -73,13 +73,23 @@
class))))
p))

+(defn stringify-class [{:keys [class] :as props}]

• (if (coll? class)

I have used coll? in order to support sets actually (which is not covered
by tests as I see it now, bummer). Do you think it's
impractical/unnecessary?

—
Reply to this email directly or view it on GitHub
https://github.com/reagent-project/reagent/pull/154/files#r43231780.

[mike-thompson-day8]

@holmsand are you okay with this being merged? It has tests but no docs.

[danielcompton]

It would be good to add tests for sets if that usage is going to be supported.

[zoldar]

Added test for sets and a short mention of the feature in README.md

EDIT: Oops, hit some conflicts. Will try to sort it out later.

[holmsand]

@zoldar Sorry about that – I've just merged a whole lot of code all over the place :)

I think this patch is a good idea btw, so we should merge it.

[Deraen]

Maps where mentioned in some comment. I can think a way how maps could be useful for setting classes:

;; active? true, odd? false
:class [(if active? "active") (if odd? "odd")]
:class {:active active? :odd odd?}
;; => "active"

;; active? true, odd? true
:class [(if active? "active") (if odd? "odd")]
:class {:active active? :odd odd?}
;; => "active odd"

So map keys with truthy values would be used as class names, and if needed converted to strings.

[bsteuber]

I like that way of using maps - it's like Angular's ng-class directive and
would in indeed be useful for conditionals.

On Fri, Oct 30, 2015 at 12:33 PM Juho Teperi [email protected]
wrote:

Maps where mentioned in some comment. I can think a way how maps could be
useful for setting classes:

;; active? true, odd? false
:class [(if active? "active") (if odd? "odd")]
:class {:active active? :odd odd?};; => "active"
;; active? true, odd? true
:class [(if active? "active") (if odd? "odd")]
:class {:active active? :odd odd?};; => "active odd"

—
Reply to this email directly or view it on GitHub
#154 (comment)
.

[mike-thompson-day8]

@Deraen I'm not a fan of the map version. Too much magic, for not enough benefit.

By magic I mean: an experienced clojurescript developer is not going to immediately understand what's going on without some coaching. There'd have to be a HUGE upside to warrant the overhead of puzzling people. Remember this is a relatively minor, non essential, convenience feature.

[zoldar]

I'm with @mike-thompson-day8 on the map suggestion. That's a bit too much magic for me too.

P.S.: Resolved the conflicts.

[mike-thompson-day8]

@holmsand are you still okay with this being merged?

[holmsand]

In principle yes. But first I'd like to make sure that it doesn't have any adverse effects on performance. I think it can be made quite fast, but I'd like to (finally) get a 0.6.0-alpha out the door first, this week I sincerely hope...

[ericnormand]

Ping! Any idea when this will be released?

[aerosol]

@holmsand ping

[GetContented]

Suggestion: improve language.

"The :class attribute can accept either a collection or a string"

[zoldar]

Applied feedback and rebased.

[zoldar]

[Deraen]

Looks good. I was waiting for #313 before merging some PRs, as I broke CI config at some point, but as this has good tests, I'm sure this will work after I merge new test runner.

[pesterhazy]

Fantastic to see this land - a much requested feature!

[MichaelFHScott]

I can't seem to make this work - it separates the classes with commas not spaces so they aren't recognised. Any idea what I'm doing wrong?

[ericnormand]

@MichaelFHScott Can you share a quick snippet of code so we can see what you're doing?

[MichaelFHScott]

Thanks for the very quick response. It was my stupid mistake - I had accidentally reverted to Reagent 0.7.0.
I'm just discovering Clojurescript and Reagent. It's brilliant!