Don't ignore values.class when string type

[bloodyKnuckles]

The lodash.union function ignores arguments that are string type, therefore values.class of type string are not added to the classes array, and don't end up in the resulting HTML string.

First look at the lodash union function instantiation: https://github.com/lodash/lodash/blob/master/dist/lodash.js#L7579

The pertinent argument (last one in this version) is true.

Now look at the baseFlatten function, where a string is handled when union'izing: https://github.com/lodash/lodash/blob/master/dist/lodash.js#L2613-2614

...the corresponding argument (isStrict) must be false in order for strings to be included in the resulting array. Therefore strings are ignored.

The reason for using values.class && [values.class] and not just [values.class] is because when values.class is undefined and wrapped in array brackets it still registers as an element on the array, so having only [values.class] will always produce the class attribute in the HTML string, but empty when values.class === undefined (i.e., class="").

Example: http://esnextb.in/?gist=0512c49748550e57039bce437225081e

const vdom = sh('div.wrap', {attrs: {'class': 'one'}}, [sh('div#box', 'contents')])
const vdom1 = sh('div', {attrs: {'class': 'one'}}, [sh('div#box', 'contents')])

console.log(v2h(vdom))
console.log(v2h(vdom1))

With vdom, class: one is lost

The reason vdom1 works is because when classes is a zero length array values.class is not overwritten, see https://github.com/acstll/snabbdom-to-html/blob/master/src/modules/attributes.js#L36-L38

[acstll]

Thank you @bloodyKnuckles, I will read your comment and PR through during the day. 👍

[acstll]

Your PR (thanks again for it) made me discover two important things:

In your example, if you use snabbdom to render vdom, it's actually the wrap class that gets lost. (Please try it yourself). The attributes module calls setAttribute on the DOM element, and so elm.setAttribute('class', 'one') overrides the previous class value on the element. This is OK, so the current "merging" behaviour in this library might not be totally correct.

The second thing, which is a real problem, is that in the attrs tests I used the className key, which is just wrong. See https://github.com/acstll/snabbdom-to-html/blob/master/test/index.js#L134 Using className inside the attrs objects yields a classname attribute set on the DOM element (like elm.setAttribute('className', 'foo')). Definitely not what you want.

Besides from sticking to the HTML spec, matching snabbdom's behaviour 1:1 is really important.

I could merge you PR now and cut a release (a major bump?), but I think the issues exposed above should be addressed too, which would almost surely mean some refactoring in attributes.js. What do you think would be best @bloodyKnuckles, merge, release, and then reactor, or just wait and refactor to address all issues?

[bloodyKnuckles]

I suppose what size steps to take depends on the size of the tasks. If all these issues are relatively small then get them all done then merge. If there's some bigger tasks, split them out.

I'm glad you found more issues. I intend to dig in today and see if I can help with those too.

[bloodyKnuckles]

I've updated the test code with DOM render: http://esnextb.in/?gist=0512c49748550e57039bce437225081e

I've added a stylesheet and changed up the class names to correspond to the stylings to better help see what's there and what's missing.

I've also made a pull request on snabbdom to prevent overwriting the sel subcomponent class: https://github.com/paldepind/snabbdom/pull/117

[acstll]

Nice. I'll create some separate issues and a 3.0 todo list (including this PR) as soon as I get a chance.

I'll also want to wait for a decision on paldepind/snabbdom#117 because I think the current overriding behaviour could be correct (by design).

Thanks again!

[bloodyKnuckles]

https://github.com/paldepind/snabbdom/pull/117#issuecomment-221351841

You are correct, in snabbdom, attrs class is intended to overwrite sel subcomponent, rather than augment.

So I updated this PR with that in mind: https://github.com/acstll/snabbdom-to-html/pull/13/files

Changed to:

classes = union(classes, values.class && [values.class] || parsedClasses).filter(x => x !== '')

So union now only gets two arguments: classes and EITHER classes derived from attrs, OR, if that does not exist, then classes derived from the sel property.

values.class && [values.class] || parsedClasses

First check to see if values.class is defined, if so wrap in array brackets (so lodash.union doesn't ignore it) and return that. If values.class is NOT defined, then provide parsedClasses.

[acstll]

Thanks again, @bloodyKnuckles I'll keep this on the list for 3.0.

[acstll]

Hey there, I just ran npm test agaist this PR and I got some failings, mainly this (an extra comma):

expected: |-
      '<a id="github" class="a b" href="http://github.com" target="_blank">Github</a>'
    actual: |-
      '<a id="github" class="a,b" href="http://github.com" target="_blank">Github</a>'

Since I will change the merging behaviour of the library (paldepind/snabbdom#117) and some of the tests, I'm going to close this for now. I will however take it into account while doing the changes.

Thanks again, I appreciate it 👍

[acstll]

Fixed in 2771b7f07fd120ab0e89a537f109853e152fc6e6 :-)