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

Add extend method to Classes #626

Merged
merged 4 commits into from
Sep 5, 2019
Merged

Conversation

hgzimmerman
Copy link
Member

@hgzimmerman hgzimmerman commented Sep 1, 2019

Also implement Clone.

Addresses #621


I'm a little off-put by the use of format!() having to be used to concatenate strings that are later used as classes.

This change allows for usages like:

html!{
    <div class=Classes::new().union("some-class").union("other-class") />
}

or

html!{
    <div class=self.class.clone().union("other-class") />
}

@hgzimmerman
Copy link
Member Author

Random thought: maybe union might be a better name given that that is actually what we want to accomplish?

@hgzimmerman hgzimmerman changed the title Add extend method to Classes Add union method to Classes Sep 1, 2019
@davidkna
Copy link
Contributor

davidkna commented Sep 1, 2019

I think extend is better both to avoid confusion because the behaviour of your function aligns better with IndexMap::extend than IndexMap::union and also because makes it clearer it works with single (String) values.

@hgzimmerman
Copy link
Member Author

Yeah, I'm flip-floping hard on this minor detail. I think extend as the function name with comments for that and push saying that it takes the logical union of the two sets of classes is the best course of action.

@hgzimmerman hgzimmerman changed the title Add union method to Classes Add extend method to Classes Sep 1, 2019
@serzhiio
Copy link
Contributor

serzhiio commented Sep 2, 2019

Maybe something like this is possible for .new() constructor?
Classes::new(&["grid","wrapper","main"])
or
Classes::from(&["grid","wrapper","main"])
Imo, it looks a bit easier to work with...

@hgzimmerman
Copy link
Member Author

We already have Classes::from("grid wrapper main"), which is effectively what the framework uses when you specify the class attribute inside of the html! macro.

What this PR is targeting is the scenario where you want to pass Classes around as props, but don't want to use format! when you want to conditionally add another class to the set.

@serzhiio
Copy link
Contributor

serzhiio commented Sep 2, 2019

Great, i didnt know about that! Missed in docs :(

@@ -17,6 +17,7 @@ pub use self::vnode::VNode;
pub use self::vtag::VTag;
pub use self::vtext::VText;
use crate::html::{Component, Scope};
use std::ops::RangeFull;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove

@jstarry
Copy link
Member

jstarry commented Sep 5, 2019

bors r+

bors bot added a commit that referenced this pull request Sep 5, 2019
626: Add extend method to Classes r=jstarry a=hgzimmerman

Also implement Clone.

Addresses #621

----
I'm a little off-put by the use of `format!()` having to be used to concatenate strings that are later used as classes.

This change allows for usages like:
```rust
html!{
    <div class=Classes::new().union("some-class").union("other-class") />
}
```
or 
```rust
html!{
    <div class=self.class.clone().union("other-class") />
}
```


Co-authored-by: Henry Zimmerman <zimhen7@gmail.com>
@bors
Copy link
Contributor

bors bot commented Sep 5, 2019

Build succeeded

  • continuous-integration/travis-ci/push

@bors bors bot merged commit 7ae0c0e into yewstack:master Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants