Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: Add support of optional class names #936

Closed
liquidnya opened this issue Feb 10, 2020 · 6 comments · Fixed by #1085
Closed

Feature request: Add support of optional class names #936

liquidnya opened this issue Feb 10, 2020 · 6 comments · Fixed by #1085
Labels

Comments

@liquidnya
Copy link
Contributor

Is your feature request related to a problem? Please describe.
When using multiple class names, i sometimes want to add optional classes (of type Option<String>):

fn view(&self) -> Html {
    let color: &Option<String> = &self.color; // e.g. Some("blue".to_string())
    html! {
        <div class=("ui", color.as_ref().map(String::as_ref).unwrap_or(""), "game", "card",)>
            { &self.text }
        </div>
    }
}

Describe the solution you'd like
I would love to be able to use &str, String, Option<String> (and similar) together:

// solution 1
fn view(&self) -> Html {
    let color: &Option<String> = &self.color;
    html! {
        <div class=("ui", color, "game", "card",)>
            { &self.text }
        </div>
    }
}

This, of course, does not work with Vec<&str>, nor Vec<T> where T: AsRef<str>:

impl<T: AsRef<str>> From<Vec<T>> for Classes {
)
But the html! macro does not require for a vector of same types and the code even refers to it as tuple:
ClassesForm::Tuple(classes) => quote! {
#vtag.add_classes(vec![#(&(#classes)),*]);

Describe alternatives you've considered
An alternative, would be to allow for a vector of optional values to be converted into Classes.
E.g.

// solution 2
fn view(&self) -> Html {
    let color: &Option<String> = &self.color;
    html! {
        <div class=(Some("ui"), color, Some("game"), Some("card"),)>
            { &self.text }
        </div>
    }
}

All solutions above have the problem, that it might not be possible to detect if an element is of type Option<T> or of type T where T: AsRef<str> in both cases.

E.g. consider:

impl<T: AsRef<str>> From<Vec<Option<T>>> for Classes

which is not possible, as far as i know, due to E0119:

note: upstream crates may add a new impl of trait `std::convert::AsRef<str>` for type `std::option::Option<_>` in future versions

Another way to archive this feature would be to combine this feature request with #903 and use class? with Option<String>, Option<&str> and Vec<Option<T>> (instead of Option<Vec<T>> where T: AsRef<str>) instead of Option<Classes>.
E.g.

// solution 3
fn view(&self) -> Html {
    let color: &Option<String> = &self.color;
    html! {
        <div class?=("ui", color, "game", "card",)>
            { &self.text }
        </div>
    }
}

Maybe there is another way to archive a solution to this problem.
E.g. by not using AsRef<str>, but rather implement versions for Vec<String> and Vec<&str>, but that would be a breaking change and AsRef<str> might be more desirable than this proposed feature. (solution 4)

@liquidnya liquidnya added the feature-request A feature request label Feb 10, 2020
@jstarry
Copy link
Member

jstarry commented Feb 11, 2020

Agreed that this would be a great feature! I was just talking in the gitter chatroom about Yew's api being more flexible in general... more in line with this: https://deterministic.space/elegant-apis-in-rust.html#intooption_

@liquidnya
Copy link
Contributor Author

Agreed that this would be a great feature! I was just talking in the gitter chatroom about Yew's api being more flexible in general... more in line with this: https://deterministic.space/elegant-apis-in-rust.html#intooption_

This document is very helpful.
Maybe Into<Option<_>> (https://deterministic.space/elegant-apis-in-rust.html#intooption_) can be used to easily solve the problem with E0119. (solution 1 and 2 in my feature request)

@jstarry
Copy link
Member

jstarry commented Feb 11, 2020

Maybe Into<Option<>> (https://deterministic.space/elegant-apis-in-rust.html#intooption) can be used to easily solve the problem with E0119. (solution 1 and 2 in my feature request)

You read my mind 👍

@liquidnya
Copy link
Contributor Author

All solutions above have the problem, that it might not be possible to detect if an element is of type Option<T> or of type T where T: AsRef<str> in both cases.

note: upstream crates may add a new impl of trait std::convert::AsRef<str> for type std::option::Option<_> in future versions

I tried to use AsRef<str> together with Into<Option<_>> (https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=33026ababfd4b4b49579d9b9e1fa0f9d) and the compiler needs some type hints (E0282).
I might overlook something or i might approach the problem the wrong way.

Maybe the problem could be solved with specialization, since the input could either be Option<T> or T where T: AsRef<str> respectively.
Unfortunately specialization is not available in Rust as of now: rust-lang/rust#31844
But by following this issue to rust-lang/rfcs#1210, i found this linked issue: seed-rs/seed#268, which mentions this workaround for specialization:
https://github.com/dtolnay/case-studies/blob/master/autoref-specialization/README.md

It describes a way to use autoref for specializations within macros only.
Since html! is a macro, this neat trick could be used to allow for Option<T> and T where T: AsRef<str> respectively.

seed-rs/seed#268 did something very simmilar:

So let say we want style! to support every type that impl ToString as seed already do, and support any Option where T: ToString this last one is the new thing we want to add to style! macro.

Maybe this method also allows for passing Iterators (or T where T: IntoIter), Vectors, T where T: AsRef<str>, etc. to class.
Maybe not even limited to the class property.
This would allow the usage for various supported types without the need to convert them manually.

Maybe this method could also be applied to #903, enabling the use of optional href=None without the need of new syntax (href?=None), if desired.

@jstarry
Copy link
Member

jstarry commented Apr 25, 2020

@LiquidBlock that workaround sounds promising! Is that something you're interested in tackling?

@liquidnya
Copy link
Contributor Author

I was looking into it and i had something that worked to some extend, but I also run into some problems.
Maybe at least adding Option<T> where T: AsRef<str> would be a possibility, see #1085 (comment).

I am not so sure if specialization (even with the workaround) does the trick for allowing both Option<T> and T where T: AsRef<str> respectively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants