search

weavejester / hiccup

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

Fix keyword class names not converted to strings #170

Closed mjmeintjes closed 2 years ago

mjmeintjes commented 4 years ago

When classes is nil, the merge code that converts keyword class names to strings is not being called. This commit ensures that the code gets called if there are any classes.

weavejester commented 4 years ago

Thanks for the PR. Can you remove the extraneous commits?

mjmeintjes commented 4 years ago

Sure. I see I also forgot the deps.edn file in the commit - are you ok if I leave it in or do you prefer I take it out?

weavejester commented 4 years ago

Please leave it out.

mjmeintjes commented 4 years ago

I've squashed the commits and removed the deps.edn commit.

weavejester commented 4 years ago

You still have the str/trim change. Can you remove that as well?

You'll also need to add in some tests.

mjmeintjes commented 4 years ago

You still have the str/trim change. Can you remove that as well?

You'll also need to add in some tests.

I've added a test.

I included the str/trim to prevent having to change existing tests, as without it 20 tests fail.

Eg without str/trim:

FAIL in (auto-escaping) (core_test.clj:182)
elements
expected: (= (str (html [:p {:class ["<\">"]}])) "<p class=\"&lt;&quot;&gt;\"></p>")
  actual: (not (= "<p class=\" &lt;&quot;&gt;\"></p>" "<p class=\"&lt;&quot;&gt;\"></p>"))
weavejester commented 4 years ago

What's causing the failure when str/trim is omitted?

mjmeintjes commented 4 years ago

If classes is nil (ie if the element name is bare and contains no "sugared" classes), then merge-classes will prepend a leading space without the str/trim.

All the existing tests assume that merge-classes won't even be called when classes is nil, with with the new change in this PR, merge-classes gets called even if classes is nil. So the tests either need to be updated, or the resulting class string needs to be trimmed. I prefer going with the trim version as it does not effect existing output, and it looks cleaner.

weavejester commented 4 years ago

Ah, okay, now with the tests I see what you're doing. I don't think we want to have special behavior for the classes attribute in particular. Instead, lets change this line to make all keywords render as strings when used in attributes.

mjmeintjes commented 4 years ago

I've pushed a change to make all keywords render as strings when used in attributes. It is a bit more than one line change, because I assume that you want to support integers and other non-Named values in attributes.

weavejester commented 4 years ago

There's already a protocol function, hiccup.util/to-str that will convert objects to strings.

mjmeintjes commented 4 years ago

I've changed the code to use the hiccup.util/to-str function.

weavejester commented 4 years ago

You should now be able to remove the other changes as well, as they shouldn't be necessary.

mjmeintjes commented 4 years ago

Done, now only a couple of characters change.

Reminds me of this story.

weavejester commented 4 years ago

Thanks. Let's fix the commit message next so that it reflects the content of the commit. Can you change the commit message to:

Map sequential attribute values through to-str
weavejester commented 2 years ago

Closed by #195.