Closed zerg000000 closed 7 years ago
Thanks for the commits. I've added some comments in line. Might be a while before I get around to merging this because I haven't focused on Hiccup for a while, but it might be time for me to get back into working on it.
Hi @weavejester ,
any further changes are needed to merge to pull request?
Yep, you still need to address one of my comments: https://github.com/weavejester/hiccup/pull/139#discussion_r92187680
Also I usually abbreviate clojure.string
to str
. It's the convention I use in the rest of the Hiccup source, so it would be good to adopt it in the compiler namespace as well.
The code:
(str
(->> value
(mapv (fn [[k v]] (str (util/as-str k) ":" v)))
sort
(string/join ";"))
";")
Could be spun off into a separate function for clarity, and we could write it a little better as:
(->> value
(map (fn [[k v]] (str (util/as-str k) ":" v ";")))
(sort)
(apply str))
hi @weavejester , could you help to review this changes? should be completed.
At a glance it looks fine, but I won't be able to properly review and merge it until January.
noted. ;-)
This has been a much wanted feature, so thanks for the work!
This looks fine. Can you squash down the commits? Perhaps with a commit message like:
Add syntax sugar for class and style attributes
Yes. Please check.
p.s. first time to do squash commits.
Looks fine. Merged. :)
Wonderful, thanks @zerg000000 and @weavejester for making this happen!
Which version of Hiccup supports this? I've been using "1.0.5" as stated on the README, but that version does not support style map rendering.
@theronic Try 2.0.0-alpha1
Hello @weavejester. First of all, thanks for your work on this.
Unfortunately, I tried switching to both 2.0.0-alpha1
and 2.0.0-aplha2
(confirming versions with lein deps :tree
), but wasn't able to get this to work. Am I missing something?
Thanks
Could you give a little more information, @metasoarous? Can you replicate the following results:
user=> (require '[hiccup.core :as h])
nil
user=> (h/html [:span.foo {:style {:background-color "red" :color "red"}} "text"]))
"<span class=\"foo\" style=\"background-color:red;color:red;\">text</span>"
@weavejester I think the reason this didn't work for me is that I was in a checkout project, and as a result the environment wasn't getting built properly. At least that's my best guess at the moment. I had some other dependencies misbehaving which suddenly now are fine, after moving the project out of the checkouts directory and re-cleaning things. It's either that or ghosts. In any case, I have now verified that the intended behavior does indeed now work, so I'm good to go. Thanks for hearing me out and taking the time to respond.
add code to support attribute value is map or vector.
Case: vector
Case: map