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

Thoughts on improving style! macro. #268

Closed
MuhannadAlrusayni opened this issue Oct 29, 2019 · 3 comments
Closed

Thoughts on improving style! macro. #268

MuhannadAlrusayni opened this issue Oct 29, 2019 · 3 comments

Comments

@MuhannadAlrusayni
Copy link
Contributor

MuhannadAlrusayni commented Oct 29, 2019

Hi.
I have been experimenting on ways to let style! macro accept more types that can be some how converted to CSSValue.

Here is how I want to use style! macro (from #261 (comment)):
Here is a model Flex:

pub struct Flex<Msg: 'static + Clone> {
    node: Node<Msg>,
    // flex properties
    inline: bool,
    direction: Option<Direction>,
    wrap: Option<Wrap>,
    justify_content: Option<JustifyContent>,
    align_items: Option<AlignItems>,
}

and when I want to view this in the view function, I write this in today seed:

    fn view(&self) -> Node<Msg> {
        // style
        let style = style![
            St::Display => if self.inline { "inline-flex" } else { "flex" },
            St::FlexDirection => self.direction.map_or(CSSValue::Ignored, |dir| dir.into()),
            St::FlexWrap => self.wrap.map_or(CSSValue::Ignored, |wrap| wrap.into()),
            St::JustifyContent => self.justify_content.map_or(CSSValue::Ignored, |justify| justify.into()),
            St::AlignItems => self.align_items.map_or(CSSValue::Ignored, |align| align.into()),
        ];

        // omited ..
    }

what I really would like to write is:

    fn view(&self) -> Node<Msg> {
        // style
        let style = style![
            St::Display => if self.inline { "inline-flex" } else { "flex" },
            St::FlexDirection => self.direction,
            St::FlexWrap => self.wrap,
            St::JustifyContent => self.justify_content,
            St::AlignItems => self.align_items,
        ];

        // omited ..
    }

Note that FlexDirection, FlexWrap, JustifyContent and AlignItems all implement Display trait.

So the code would be simple to write and read if style! macro support this kind of sugar.


The way macro style! work is to accept any type that implement Into<CSSValue>.

The problem is that Rust today only accept one blanket implementation, and since there is a blanket implementation Impl<T: ToString> From<T> for CSSValue, we cannot add another blanket implementation such as:

impl<T: ToString> From<Option<T>> for CSSValue {
    fn from(source: Option<T>) -> Self {
        unimplemented!()
    }
}

This blanket impl would allow style! macro to accept values such as Some("2px") and Some(FlexDirection::Row) ..

But as you can see this is not supported in Rust today (without specialization at less).


So I went a head and experimenting with specialization (rust-lang/rfcs#1210), but I give up because it's not ready, even if it's ready I don't think that we need to use nightly features in seed.

After that I was thinking to experimenting on the type system and macro to achieve this feature.

after a few searches I found this cool trick/workaround/idea (or what ever they call it) to do the job for us. and I have tried it and it worked!.

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

Here is a full example doing that:

use seed::prelude::*;

pub trait ToCssValueForToString {
    fn to_css_value(self) -> CSSValue;
}

pub trait ToCssValueForOptionToCssValue {
    fn to_css_value(self) -> CSSValue;
}

impl<T: ToString> ToCssValueForToString for T {
    fn to_css_value(self) -> CSSValue {
        CSSValue::Some(self.to_string())
    }
}

impl<T: ToCssValueForToString> ToCssValueForOptionToCssValue for Option<T> {
    fn to_css_value(self) -> CSSValue {
        self.map_or(CSSValue::Ignored, |v| v.to_css_value())
    }
}

macro_rules! style {
    { $($key:expr => $value:expr $(;)?$(,)?)* } => {
        {
            let mut vals = IndexMap::new();
            $(
                vals.insert($key.into(), (&$value).to_css_value());
            )*
                seed::dom_types::Style::new(vals)
        }
    };
}


fn main() {
    let width: Option<u32> = None;
    let height = Some(100u32);

    let style = style!{
        St::Display => "flex",
        St::Height => width,
        St::Height => height,
    };

    println!("{:?}", style);
}

This workaround have some limitation which I don't think they are very important in our case.

Note: This improvement can be applied to attrs! macro too.

I wrote this long issue to know if this solution is valid and would be accepted if I make PR for it. Also I would love to know if you guys have better ideas.

Thanks for reading.

@MartinKavik
Copy link
Member

MartinKavik commented Oct 30, 2019

Looks good!
It should be also applicable to attrs and possibly iterators (Seed only supports Map iterator in macros due to specialization).

@David-OConnor
Copy link
Member

Love it

@MuhannadAlrusayni
Copy link
Contributor Author

Closing since this is now solved.

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

No branches or pull requests

3 participants