search

weavejester / hiccup

Fast library for rendering HTML in Clojure
http://weavejester.github.io/hiccup
Eclipse Public License 1.0
2.68k stars 174 forks source link

Fixes #143 tag classes combined with non-literal class attributes #151

Closed pieter-van-prooijen closed 6 years ago

pieter-van-prooijen commented 6 years ago

Hello James,

This PR is a left-over of the Dutch Clojure group bug hunt session. It defers the construction of the class tag of an element till runtime if the class value is a symbol It would otherwise evaluate the symbol at compile time if there are also classes specified on the tag name, throwing an error. I'm not sure if there are other cases where this behaviour could also happen.

weavejester commented 6 years ago

Thanks! Can you tweak the commit message so that it follows the seven rules?

pieter-van-prooijen commented 6 years ago

Hi, I've updated the commit message so it states the issue more clearly

weavejester commented 6 years ago

Thanks! Can you change the subject line of the commit to:

Fix combined dot-notation and non-literal classes
weavejester commented 6 years ago

However... now that I look at the code more closely, I'm afraid I don't think this is fully correct. The problem is that merge-classes is used by both render-element (which renders at runtime) and compile-element (which renders at compilation).

Essentially I think we need to add a compilation-only version for normalize-element. We keep the runtime version, since that's public, but we split off the common functionality into private functions.

pieter-van-prooijen commented 6 years ago

I've made an internal normalize-element-with-compile-mode function which takes an extra compiling? argument to control the behaviour of the class name merging. Normalize-element now assumes it's rendering an element (not compiling) as this was the original behaviour. The only other change is that quoted symbols used in rendering mode will render with their name as the class name.

pieter-van-prooijen commented 6 years ago

Hello James, I've made the changes you suggested. Note that merge-classes now has to be public, because it can be called outside of the compiler name space.

weavejester commented 6 years ago

Thanks! merge-classes being public is fine; the compiler namespace is intended to be mostly internal anyway. Sorry for taking some time to get around to reviewing this.

pieter-van-prooijen commented 6 years ago

Hi James, I've removed the doc strings and whitespace

weavejester commented 6 years ago

Thanks! Merged.