plumatic / dommy

A tiny ClojureScript DOM manipulation and event library
759 stars 74 forks source link

Support nested seqs in tag contents/children #10

Closed davesann closed 11 years ago

davesann commented 11 years ago

Please advise if the following change is of interest to you:

Modified to allow the following:

[:parent-tag {...} (([:tag1] [:tag2]) [:tag3])]

to be equivalent to

[:parent-tag {...} [:tag1] [:tag2] [:tag3]]

This is useful when building up data from various functions where you don't want to wrap children in [:div] or flatten/concatenate lists at each stage. This is consistent with behaviour in crate and hiccup. Effectively seqs (not vectors) are looked-through for elements that can be rendered.

I added this because, when I tried dommy, I already had code that failed without it.

Timing for me indicates that this may be faster than the special case with mapcat in the current version. Please verify for yourselves.

Also:

aria42 commented 11 years ago

The current code will flatten the sequence once to handle cases like [:div ([:span "a"][:span "b"][:span "c"])]. Even that I'm not a huge fan of but there's too many cases of things like

   [:ul (for [e elem] [:li e])]

If you want to do something like this example you should wrap it in a div tag I think.

davesann commented 11 years ago

I don't agree with that view. I strongly dislike the idea of introducing new tags without a good reason. How my data is constructed is not a good reason.

The use case I have is when you have chunks of tags that you want to reuse in various places, something like:

(def some-stuff (list of tags))

(def some-more-stuff some-stuff + new tag on end))

I have a choice, either I do

(def some-more-stuff (concat some-stuff [[:new tag]]))

or, I just build it up as lists and allow these to be flattened when rendering.

Personally, I prefer the latter. because I don't want to create temporary one element vectors and then concatenate them to lists.

It's up to you - at the end of the day, I have no other justification than: I use this capability, it is useful to me; It may be to others; as far as I can see there is no detrimental effect and it may be faster than the mapcat special case you use now.

If you prefer not to include this - please feel free to close the pull request.

mpenet commented 11 years ago

+1 I encountered the same issue when porting code from crate to dommy (a select element with a default option tag with a sequence from a map call next to it). It also allows to use array like objects that are sequential (like say a jquery object/collection from jayq) directly, which is also something crate supports.

aria42 commented 11 years ago

Ok. I do think I want to support this. I like append-children and want to do something like that. Some of the other things in the commit I'm not so sure about: I don't see why js/Array.prototype.slice is wrong, nor do I think there's a point to unchecked-node rather than call the protocol directly, or the throw- fn, but these are stylistic.

Can you either give me a new pull request with just append-children or I Can just take your commit and make the stylistic changes

On Thu, Feb 7, 2013 at 12:54 AM, Max Penet notifications@github.com wrote:

+1 I encountered the same issue when porting code from crate to dommy (a select element with a default option tag with a sequence from a map call as siblings). It also allows to use array like objects that are sequential (like say a jquery object/collection from jayq) directly, which is also something crate allowed.

— Reply to this email directly or view it on GitHubhttps://github.com/Prismatic/dommy/pull/10#issuecomment-13226943.

website: http://aria42.com

davesann commented 11 years ago

Yep, I will give you an updated pull request if you like.

js/Array.prototype.slice was "wrong" only because of this : https://groups.google.com/d/topic/clojure/rtmPtmpo4qA/discussion

http://dev.clojure.org/jira/browse/CLJS-455

So I was seeing warning in my build. but it looks like this position may be reversed.

aria42 commented 11 years ago

Cool, I'll get that merged today then.

On Thu, Feb 7, 2013 at 3:21 PM, davesann notifications@github.com wrote:

Yep, I will give you an updated pull request if you like.

js/Array.prototype.slice is "wrong" only because of this : https://groups.google.com/d/topic/clojure/rtmPtmpo4qA/discussion

— Reply to this email directly or view it on GitHubhttps://github.com/Prismatic/dommy/pull/10#issuecomment-13268213.

website: http://aria42.com

aria42 commented 11 years ago

Also, if you could add a test case it would be much appreciated :)

On Thu, Feb 7, 2013 at 3:24 PM, Aria Haghighi me@aria42.com wrote:

Cool, I'll get that merged today then.

On Thu, Feb 7, 2013 at 3:21 PM, davesann notifications@github.com wrote:

Yep, I will give you an updated pull request if you like.

js/Array.prototype.slice is "wrong" only because of this : https://groups.google.com/d/topic/clojure/rtmPtmpo4qA/discussion

— Reply to this email directly or view it on GitHubhttps://github.com/Prismatic/dommy/pull/10#issuecomment-13268213.

website: http://aria42.com

website: http://aria42.com

aria42 commented 11 years ago

Any update on that pull req?

On Thu, Feb 7, 2013 at 3:21 PM, davesann notifications@github.com wrote:

Yep, I will give you an updated pull request if you like.

js/Array.prototype.slice is "wrong" only because of this : https://groups.google.com/d/topic/clojure/rtmPtmpo4qA/discussion

— Reply to this email directly or view it on GitHubhttps://github.com/Prismatic/dommy/pull/10#issuecomment-13268213.

website: http://aria42.com

davesann commented 11 years ago

I'll get it to you today. I didn't get time yesterday.