Few improvements for Classes

[cecton]

Description

1. push, contains, extend

The API of Classes is inconsistent with the standard API because it uses a custom function extend() instead of the trait Extend.

But then I realized that the functions push() and contains() could also be improved by accepting more type of objects.

This is a sample code from yewprint where you can see better the improvement it will bring:

Currently:

        let mut class = Classes::from("bp3-button");
        if self.props.fill {
            class.push("bp3-fill");
        }
        if self.props.minimal {
            class.push("bp3-minimal");
        }
        class = class.extend(&self.props.intent);

I'm using extend here instead of push because self.props.intent is an Option<Intent>.

With the modifications I'm proposing this becomes possible:

        let mut class = Classes::from("bp3-button");
        if self.props.fill {
            class.push("bp3-fill");
        }
        if self.props.minimal {
            class.push("bp3-minimal");
        }
        class.push(self.props.intent);

This is possible because my object Intent implements Into<Classes> (via From).

It also allows me to use contains because Intent implements AsRef<str>:

    if class.contains(Intent::Primary) {
        ...
    }

2. &'static str instead of String

I also added a new constructor new_static() that allows using &'static str instead of String as internal type to reduce allocations.

The reasoning is that a single class is generally not a "composed" string of other strings. Usually all the existing classes are known by the program beforehand.

The constructor new() is unchanged and still allows String to be anything.

Example with yewprint where we know all the bp3-* classes in advance. Nothing is created dynamically during runtime. Therefore all the String allocations are extra work for nothing.

Checklist

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

[siku2]

The Classes API has been bothering me for quite some time now. A lot of types in Yew's public API have the problem that they're only really optimized for use in the macro.
Thanks for taking the time to update it!

[siku2]

Just a few things I'd like to adjust.

To fix the errors in the futures example, just replace the add_class function in markdown.rs with this:

fn add_class(vtag: &mut VTag, class: impl Into<Classes>) {
    let mut classes: Classes = vtag
        .attributes
        .iter()
        .find(|(k, _)| *k == "class")
        .map(|(_, v)| Classes::from(v.to_owned()))
        .unwrap_or_default();
    classes.push(class);
    vtag.add_attribute("class", classes.to_string());
}

[siku2]

Ah, I forgot one last thing. We need to implement From<Cow<'static, str>> for Classes, otherwise the new Extend implementation won't work for Classes:

impl From<Cow<'static, str>> for Classes {
    fn from(t: Cow<'static, str>) -> Self {
        match t {
            Cow::Borrowed(v) => Self::from(v),
            Cow::Owned(v) => Self::from(v),
        }
    }
}

[cecton]

I gotta go but please don't merge because the TestCase is not code. The developer should have to define multiple times the static string that match the object.

[siku2]

Apart from the tests you mentioned this should be good to go!

One thing I realised is that we've kind of come full circle. The push method is now mostly just an alias for extend 😄
I don't think that's a problem though. push offers better ergonomics for single items on top of the extend functionality.

Pull request has been modified.

[siku2]

Yes, the syntactic sugar for classes only works for tags right now, but we can make it possible for the following to work:

struct MyComponentProps {
  pub class: Classes,
}

html! { <MyComponent class="works" /> };

let s = "hello world".to_owned();
html! { <MyComponent class=s /> };

let classes = vec!["multiple", "classes"];
html! { <MyComponent class=classes /> };

Tuples are impossible for now because of the lack of const generics, but everything else could work.

[cecton]

Yes, the syntactic sugar for classes only works for tags right now, but we can make it possible for the following to work:

Using Transformer? Yes it would be nice!

I just pushed something to avoid defining the statics multiple times but I think it is terrible.

I gotta go again. Hopefully tomorrow I will have something that works and is nice to use at the same time. Sorry for all the time it takes but I really don't want to make it worst.

This was kinda nice. The best would have been to have only one impl to make it work.

[siku2]

This, once again, violates the rule of splitting up multiple classes... We can't statically ensure that the given class value doesn't contain multiple classes.
I'm honestly not sure either what the best way is to go about this. If you want to keep it like this we need to explicitly state this in the documentation or even mark the function as unsafe (which is kind of silly).

As I mentioned in one of my previous comments, I think it was fine before.

[cecton]

I can't find one good solution that fits all. I know you preferred the previous one but for me it's terrible to have to declare 2 times the &'static str in the code.

[cecton]

(Well, the solution with the generics actually worked better because we don't mix String and &'static str)

[siku2]

I don't understand what you mean... There was no &'static in the method signature before, your changes added that just now.
The only thing I'm kind of unhappy about with the previous implementation is that it converts it into Classes only for it to be dropped again. The compiler can surely optimize this away but it's still an eyesore.

[siku2]

(Well, the solution with the generics actually worked better because we don't mix String and &'static str)

We would've had to deal with a whole different can of worms if we continued on with generics.
Being able to mix string literals with owned strings is a good thing in my opinion. It's very rare that all of your classes are dynamically generated, very often it looks something like this: ("table", "table-cell", format!("size-{}", self.size)). This wouldn't work at all with generics unless we force the first two classes into owned strings as well.

[cecton]

You're right it does make sense to be able to do that in html!

But for my use case here outside html!: https://github.com/cecton/yewprint/blob/main/src/lib.rs#L74-L97 it's not good because I would have to have those &'static str in 2 different places.

[siku2]

This has much more overhead than AsRef<str>. I think we should keep it like it was before.

[cecton]

Yes I know it's terrible

[siku2]

Using Transformer? Yes it would be nice!

Yeah, we just need to implement this:

impl<T: Into<Classes>> Transformer<T, Classes> for VComp  {
  fn transform(from: T) -> Classes {
    from.into()
  }
}

and then you can use any type that implements Into<Classes> for the class prop.

[cecton]

This is I believe the best solution: one Classes for the macro html!() and another one to manage classes in components.

Mixing &'static str and String makes sense within the html!() macro but not at all when managing classes within components. I also couldn't find a way to make a struct convertible to Classes (because of a mix of &str and &'static str).

HTMLClasses now perfectly fits html!()'s use case and has been improved to use Cow. The new Classes replacement perfectly fit the use inside a component / for a library.

[siku2]

I'll have to think it over a bit more but I'm sorry to say that I currently don't share your enthusiasm for this approach.
I really don't like the sound of having two different Classes implementations that essentially serve the same purpose.
To be fair, I also still don't understand why you were unhappy with the previous implementation in the first place.

I also couldn't find a way to make a struct convertible to Classes

That should be fairly easy:

#[derive(Clone, Copy)]
enum MyClassEnum {
  A,
  B,
}
impl MyClassEnum {
  fn as_class(self) -> &'static str {
    match self {
      A => "class-a",
      B => "class-b",
    }
  }
}
impl Into<Classes> for MyClassEnum {
  fn into(self) -> Classes {
    Classes::from(self.as_class())
  }
}

Maybe the issue is that you have to write classes.contains(MyClassEnum::A.as_class()) instead of classes.contains(MyClassEnum::A) but that seems like a small price to pay.

Mixing &'static str and String makes sense within the html!() macro but not at all when managing classes within components.

I don't see how the situation is different for components. The same reasons for mixing String and &'static str also apply to components.

---

So to be clear, I'm not saying "no" to this outright. I'm much more interested in discussing how you would like to be able to use Classes and what's stopping you from doing it.

[cecton]

Long story short: I think you're right and this is the best solution for now.

I still think there is more to it though. If you want, we could discuss it more on the discord's channel.

[cecton]

I adapted yewprint's code to use Classes inside the html! and it now looks much better. See here.

I'm not sure if I like it. It's a lot of code inside html!... Also I still can't expose a "class" prop that works exactly the same than the standard HTML. Somehow I think it would have been better to not treat the attribute class as something special and let people manage their classes the way they want. Maybe it would be a good idea to make a flag to disable the special abilities of the attribute class? I sincerely don't know. I think it's also possible to port classNames to Rust.

Let me know what you think

[siku2]

Also I still can't expose a "class" prop that works exactly the same than the standard HTML

There's an argument to be made that the class attribute is important enough to deserve special treatment for components (much like children already does).
By handling it separately in the macro, class can behave the exact same way for tags and components.

That's definitely something we should open a new issue for!

Anyway, I'm going to merge this now. There are still things that can and should be improved regarding Classes but they deserve some more discussion.

[cecton]

There are still things that can and should be improved regarding Classes but they deserve some more discussion.

💯

I also think this PR is already a nice improvement. I'm happy with it