Extract Classes to a separate macro

[cecton]

Description

This PR will extract the code handling the classes from the macro html! and put it to its own macro classes!.

The intention is to keep the super helpful macro for class composition but move it outside so people can create libraries for yew and allow their user to use this macro.

This is not possible at the moment because it is strongly coupled to the html! macro and only for base HTML tags (a, div, span, etc...) but not for custom component.

Checklist

• I have run cargo make pr-flow
• I have reviewed my own code
• I have added tests

Related to #1588

[cecton]

Hello @siku2 👋

Here is finally another implementation for the classes that we talk about.

Since it is not possible to have the behavior that we have on "class" attributes in html! for custom components, I thought it would be maybe a good idea to actually split html! and make the part that handles classes its own macro classes!

Overall it works well and would even reduce the code size (after cleanup). But there are a few caveats:

1. I had to adapt all the tests to use the new macro. This actually means that if we want to support both ways (html! handling classes AND a new classes! macro, we would need to duplicated most of the tests. Sad.
2. There is no way for the attribute class to stay unset like before. Since classes is now a string, if you pass an empty Classes (or an empty str/String), it will become class="" at the end. I think the only way to avoid that would be to make an exception in html! for the class attribute but obviously it would re-open the main reason why this PR exists: having a consistent behavior for attributes class for custom components and base components. (Or maybe we could say class is not a String but an Option<String>? Just a wild idea...)

Since yew is still in version 0.x.y I think it would be okay to make a breaking change to the 0.18. The code would only force people to adapt by prefixing all their classes by classes! like this:

<div class=("a", "b") />

To

<div class=classes!("a", "b") />

Overall I think it's beneficial and it is worth the change (because the inconsistency bothers me) but you might have a different opinion. What do you think?

[cecton]

I don't pretend to have always excellent ideas :D don't hesitate to reject!

[siku2]

This isn't quite what I had in mind when I said "discussion" 🙃. My weak little heart wouldn't be able to reject this after you put in all this work...

I do like the idea of a classes! macro and I'm also generally in favour of making class's special treatment more explicit. With that being said, there are a few things I need to bring up.

The biggest issue is that this is a super duper mega breaking change. This will force basically every single Yew app to update and it's not always as simple as adding classes! as a prefix.
Such a change certainly isn't inherently bad but I feel like there has to be a very good reason for it.
In this case you say it's:

having a consistent behavior for attributes class for custom components and base components

The problem is that there isn't even a consistent behaviour across components.
Components could very well have class: int as a prop if they wanted to.
What I'm trying to get at is that components, right now, are inherently different from tags and I don't think it's healthy to pretend they're not.
That doesn't mean that I don't want for components to have access to the same convenience as tags do. I just don't think it's sensible to remove features from tags just because components don't have them.

Here are two approaches I'm comfortable with:

1. Make classes! implicit for tags so that <div class="test" /> is like <div class=classes!("test") />. The classes! macro can then be used for components. Like Add a macro for building properties outside of html! #1599 but for Classes.
2. Turn class into a keyword for components. Much like the children field this will force the class field to be of type Classes but it would mean that things like <MyComp class=("something", "else") /> just work.

Again, the purist in me is totally in favour of moving classes out of html! into another macro but I just don't think we can justify it as it stands.

[cecton]

This isn't quite what I had in mind when I said "discussion" upside_down_face. My weak little heart wouldn't be able to reject this after you put in all this work...

Oops! 😓 I didn't mean to force your hand. I felt like I needed to test a bit more my idea to see if it is even possible and how it translates. Really I didn't spend that much time on it. This is fine!

it's not always as simple as adding classes! as a prefix.

There are 2 things but nothing more:

1. adding classes! in front
2. class= might generated attributes with empty string

What I'm trying to get at is that components, right now, are inherently different from tags and I don't think it's healthy to pretend they're not.

I see your point. className has always been a special case in React too but less of a special case because it really is just a string there.

My point is only that it will make things more consistent in general. Not perfect.

That doesn't mean that I don't want for components to have access to the same convenience as tags do. I just don't think it's sensible to remove features from tags just because components don't have them.

This is a good point. It's not exactly removed either...

Turn class into a keyword for components. Much like the children field this will force the class field to be of type Classes but it would mean that things like <MyComp class=("something", "else") /> just work.

That is very bad for me. I mean the idea is good and well intended but custom components might accept multiple class props that are applied on different HTML tags (or even other components) in their own component. Here is an example with blueprint having 2 classes property: visibleItemClassName and className (inferred from IProps). For me this is a no go.

NB: I don't think copying react is a rule of thumb, I'm very happy with Yew taking its own direction and I personally see already a lot of improvement over react because of that (mostly the state management). I don't not think either that class being exactly a String is necessarily the way to go.

Make classes! implicit for tags so that <div class="test" /> is like <div class=classes!("test") />. The classes! macro can then be used for components. Like #1599 but for Classes.

I wanted to answer first before re-modifying my PR again so I don't jump from one thing to another and have a hard time to explain what happens in my head... but I have an idea 😁 similar to what you proposed.

Since class=() is just parse like a TupleExpr I guess I can make a Punctuated<Expr, Comma> out of it and make it transparently work like it use to without duplicated code.

The only difference that would still remain is that class=() will still translate to class="". But I believe simply adding a ? here like this: class?=() might help.

There might also be a point about treating any attribute that uses the (...) syntax as class-like. But I'm not sure if it is a "good" thing that we should allow. <insert meme Your-scientists-were-so-preoccupied-with-whether-or-not-they-could,-they-didn’t-stop-to-think-if-they-should>

BTW, almost entirely not related. I was wondering why we use indexmap then I realized HashSet does not keep order so I asked DDG (not google) and this is what I found:

The order in which the attributes are overwritten is not determined by the order the classes are defined in the class attribute, but instead where they appear in the CSS.

-- Source: https://stackoverflow.com/questions/1321692/how-to-specify-the-order-of-css-classes

[cecton]

Ready for review cc @siku2

[siku2]

GitHub kind of broke because a new commit was added while I was reviewing this. Let's hope it still turned out okay.
Looks like it survived. This should be the last review for the code itself 🎉 (unless the final re-review surfaces some issue I missed, of course)

[siku2]

Okay that's not really how I imagined it. The code examples are way too verbose now and it's hard to tell what to look at.

[cecton]

Okay that's not really how I imagined it. The code examples are way too verbose now and it's hard to tell what to look at.

It can be cleaned-up a bit by removing the Boolinator import where it's not used and the struct with the component itself

[cecton]

@siku2 do you have any idea for this? I an revert to a previous state if you prefer. Or do just as I said (cleanup things a bit)

[siku2]

@siku2 do you have any idea for this? I an revert to a previous state if you prefer. Or do just as I said (cleanup things a bit)

Ah sorry, I thought you were still playing around with it to see what's best. I honestly preferred the previous version because the code in the tabs should be as concise as possible (otherwise it distracts from the point of each tab). There was also possibly a misunderstanding because I was thinking about combining the list of Into<Classes> implementations into these tabs (so instead of listing all types which can be turned into Classes at the top, we use the tabs to do it). I feel like that's easier to follow than this:

This includes: &'static str, String and Cow<'static, str>,
but also: Vec<impl Into<Classes>>, &[impl Into<Classes>],
Option<impl Classes> and &Option<impl Classes>.

[cecton]

Now I understand why they want to rewrite the React's doc 😅 Apparently it gets messy because a lot of people wrote with their own style and at some point it isn't consistent.

So... I reverted the latest change and instead I added an example tab for String and another one for Array ref. I'm not sure it would make much sense with Cow... I think the Cow is mostly here to handle the fact that we mix String and literals, I don't see a use case.

Let me know what you think with this latest changes. Feel also free to correct by yourself, I really don't mind at all.

[siku2]

Now I understand why they want to rewrite the React's doc sweat_smile Apparently it gets messy because a lot of people wrote with their own style and at some point it isn't consistent.

Haha yeah, Yew's documentation is a mess of different styles.

So... I reverted the latest change and instead I added an example tab for String and another one for Array ref. I'm not sure it would make much sense with Cow... I think the Cow is mostly here to handle the fact that we mix String and literals, I don't see a use case.

Let me know what you think with this latest changes. Feel also free to correct by yourself, I really don't mind at all.

Looks good enough :)
I'll try to squeeze in another review tomorrow just to make sure I didn't miss anything important but otherwise I think we're good to go