Do not set empty classes and support option wrapped class names

[liquidnya]

Hello,
this is a follow-up PR of #927 (which resolved #926).

• I implemented setting the class string with className (https://developer.mozilla.org/en-US/docs/Web/API/Element/className) instead of setAttribute("class", ...). (Fix wrong order of classes when reordering or adding classes to the front #927 (comment))
• Also i noticed, that when using tags within the html! macro without specifying a class attribute, the attribute is still set in the DOM. I propose to not set the class name if the classes set is empty when there is no ancestor, see https://github.com/yewstack/yew/compare/master...liquidblock:set-class-name?expand=1#diff-5925cef252f837778e2d2be90760083fR217.

E.g. The following view

html! {
    <div>{ "Example" }</div>
}

results in (as of version 0.14.3):

Fixes: #936

[jstarry]

Thanks! Do you mind adding a testcase as well to make sure this doesn't happen again?

[liquidnya]

I added test cases that check with hasAttribute("class") (https://developer.mozilla.org/en-US/docs/Web/API/Element/hasAttribute) if the className was set or not.
I tested the test case with geckodriver 0.26.0.

[liquidnya]

I have found, that yew::virtual_dom::vtag::tests::supports_svg fails if the class is set to "" when the ancestor is empty.
The error output is:

wasm-bindgen: imported JS function that was not marked as `catch` threw an error: setting getter-only property "className"

className for SVG elements is read-only and deprecated, see https://developer.mozilla.org/en-US/docs/Web/API/SVGElement#Properties.
It also says Authors are advised to use Element.classList instead.
SVG Elements have the class property: https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/class.

I will quickly add a test-case, which will fail for now.

[liquidnya]

There are multiple possibilities:

1. revert back to using setProperty("class", ...) for all nodes
2. use the classList.value property (sets the list to the given value) for all nodes https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/value
3. a) use setProperty("class", ...) only for SvgElements
   b) use the classList.value property only for SvgElements

All of the above possibilities require different browser minimum versions.

[liquidnya]

I reverted the change to use the className property (the class is set using setProperty("class", ...)).
Therefore the changes in this PR are now only fixing the issues of setting class="" and adding test cases.

[jstarry]

@LiquidBlock this looks good :) I was hoping you would treat "class" as an attribute and I wasn't disappointed!

I'm now thinking we could take this one step further! What if we remove classes from VTag and instead just add "class" to attributes?

I think we would need to remove the add_class and add_classes methods from VTag but I think that's totally fine. And set_classes could set "class" in attributes.

[liquidnya]

I think we would need to remove the add_class and add_classes methods from VTag but I think that's totally fine. And set_classes could set "class" in attributes.

add_class is used in

yew/examples/crm/src/markdown.rs

Lines 45 to 47 in 731f989

	Alignment::Left => vtag.add_class("text-left"),
	Alignment::Center => vtag.add_class("text-center"),
	Alignment::Right => vtag.add_class("text-right"),

and in

yew/yew-router/examples/guide/src/markdown.rs

Lines 47 to 49 in 9f286d1

	Alignment::Left => vtag.add_class("text-left"),
	Alignment::Center => vtag.add_class("text-center"),
	Alignment::Right => vtag.add_class("text-right"),

[liquidnya]

I'm now thinking we could take this one step further! What if we remove classes from VTag and instead just add "class" to attributes?

That is a good idea to remove a lot of "duplicate" code and changing the implementation.
The classes set by the html! macro would be converted to Classes and then converted to String, which would then be inserted into the Attributes (HashMap).

add_class could be implemented by checking if the class is not already contained in the String and then appending it to the String. This would take O(n) complexity (amortized average, where n is the count of classes already in the class string), repeated calls would require quadratic time.
Keeping the IndexSet (contained in Classes) in memory (in VTag) results in O(1) (amortized average) complexity.

[jstarry]

I think we should get rid of add_class. It just adds unnecessary complexity. If you want to do fun things with classes, you can use Classes directly and then call set_classes

[liquidnya]

I dont know why it says, that i requested a review from hgzimmerman.
I am sorry, this was not intentional, maybe i clicked something by accident.

[liquidnya]

I dont know why it says, that i requested a review from hgzimmerman.
I am sorry, this was not intentional, maybe i clicked something by accident.

I guess it happened automatically, since yew-router/examples/guide/src/markdown.rs is owned by hgzimmerman.

[liquidnya]

I removed add_class and add_classes.
set_classes now sets the attribute directly.
I further added fn classes(&self) -> &str to VTag, which is a convenience method for accessing the attribute with something like vtag.attributes.get("class").unwrap_or("").

I was not sure how to fix the markdown examples:

yew/examples/crm/src/markdown.rs

Lines 45 to 47 in 731f989

	Alignment::Left => vtag.add_class("text-left"),
	Alignment::Center => vtag.add_class("text-center"),
	Alignment::Right => vtag.add_class("text-right"),

and in

yew/yew-router/examples/guide/src/markdown.rs

Lines 47 to 49 in 9f286d1

	Alignment::Left => vtag.add_class("text-left"),
	Alignment::Center => vtag.add_class("text-center"),
	Alignment::Right => vtag.add_class("text-right"),

I added a method in those example, which parses the class string into Classes, adds the classes and sets those classes back to the VTag: https://github.com/yewstack/yew/pull/1085/files#diff-3e5d43901a65fa6fb6f5cff21489390dR7-R17
I guess that this is not the best solution, but i do not really know how to rewrite that example without investing a lot of time.

[jstarry]

Great! Much cleaner :) I'm wondering now if we should remove set_classes. What do you think?

[hgzimmerman]

The guide example for the router is getting removed in another open PR, so no worries. Glad to see the new CODEOWNERS file doing it's job.

[jstarry]

Love the way this turned out! Great work. Just a few small nits and then we can get this in :)

[liquidnya]

@jstarry thank you for the detailed code review.
I pushed an overhaul.

[liquidnya]

The beta job of Travis failed with: (https://travis-ci.com/github/yewstack/yew/jobs/324520704)

The command "tar -xzf geckodriver-v0.26.0-linux64.tar.gz" failed and exited with 2 during .

Should the title of this PR be updated to signify that also optional classes are possible now?

[jstarry]

The beta job of Travis failed

That's fine, just CI flakiness :/

Should the title of this PR be updated to signify that also optional classes are possible now?

Yes, I just updated!

@jstarry thank you for the detailed code review.
I pushed an overhaul.

Looks excellent, thank you for bearing with my feedback.