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

experimental UI Library #333

Closed
MuhannadAlrusayni opened this issue Jan 14, 2020 · 15 comments
Closed

experimental UI Library #333

MuhannadAlrusayni opened this issue Jan 14, 2020 · 15 comments

Comments

@MuhannadAlrusayni
Copy link
Contributor

So I was doing some experimental UI library and I think I reach state where I can publish this library.

I didn't add any docs yet. but the code is there if anyone would like to help :D.

https://gitlab.com/muhannad_alrusayni/khalas

This is also response for:

If we want to continue with the discussion, I suggest to create a bigger example with nested components so we can compare current and proposed API and architecture properly. Then, if that example would look promising, we would need to rewrite at least real-world example and quickstart-with-webpack. After that there will be a chance to merge it into the master because it would be a really big change

in #310 (comment)

I am not sure if opening issue is the right way to tell :D.. I will close this issue after a few days. if you think this is not the right place to post about it, feel free to close the issue now.

@MartinKavik
Copy link
Member

Very nice experiment!

On the first glance:

  1. rich macro hides available methods during reading the code and I don't know how much is it compatible with IDE autocompletes or docs generators.

  2. There are some typos in names (e.g. propertie). I recommend to use a spell-checker.

  3. https://gitlab.com/muhannad_alrusayni/khalas/blob/b1f1487f5876f5508a72377e4dc76682a8558356/src/el/control/button.rs#L38 - it's pretty big Model, but only necessary State are properties mouse_over and focus (other ones are Config) - see https://github.com/evancz/elm-sortable-table/tree/1.0.1 for explanation. I'm not sure if we want to break that Elm rule that says we should separate them and make a state as small as possible.

  4. Why GitLab? :)

  5. If I remember / assume correctly, the main goals of this experiment are:

    1. Define API pro Seed libraries.
    2. Reduce boilerplate.

    => could you add at least counter example or something a little bit bigger like server_integration from the Seed's repo so we can see how many of those goals are achieved?

@MuhannadAlrusayni
Copy link
Contributor Author

MuhannadAlrusayni commented Jan 18, 2020

Very nice experiment!

Glad you like it :D

On the first glance:

1. `rich` macro hides available methods during reading the code and I don't know how much is it compatible with IDE autocompletes or docs generators.

Autocompletes in my case doesn't work (Emacs with lsp on), I think this is the case with most if not all IDEs. docs generation is fine and it can be better.

3. https://gitlab.com/muhannad_alrusayni/khalas/blob/b1f1487f5876f5508a72377e4dc76682a8558356/src/el/control/button.rs#L38 - it's pretty big `Model`, but only necessary `State` are properties `mouse_over` and `focus` (other ones are `Config`) - see https://github.com/evancz/elm-sortable-table/tree/1.0.1 for explanation. I'm not sure if we want to break that Elm rule that says we should separate them and make a state as small as possible.

hmmm, I really don't know Elm and I can't read it's code. Anyway, What would Button look like when we use Config ? and what's the benefit from doing that ?

4. Why GitLab? :)

A lot of reasons :D.. Why GitLab

5. If I remember / assume correctly, the main goals of this experiment are:
   
   1. Define API pro Seed libraries.
   2. Reduce boilerplate.
   
   => could you add at least `counter` example or something a little bit bigger like `server_integration`    from the Seed's repo so we can see how many of those goals are achieved?

sure, I will add some examples soon.

@MartinKavik
Copy link
Member

hmmm, I really don't know Elm and I can't read it's code. Anyway, What would Button look like when we use Config ? and what's the benefit from doing that ?

  • I recommend to learn Elm - you will be able to take inspiration from Elm projects and articles. And Elm is probably the simplest functional language.
  • I've tried to rewrite Radio component to standard Elm arch - see https://github.com/MartinKavik/seed/tree/radio/examples/radio/src.
  • Config is set by parent component/view - it contains business data derived from the parent's model or data based on context (e.g. hard-coded styles). State contains only internal non-business data. So the benefit is that there is a clear distinction between local state and global state + customizations. I don't know how much this is possible and wanted in your architecture.

@MuhannadAlrusayni
Copy link
Contributor Author

* I recommend to learn Elm - you will be able to take inspiration from Elm projects and articles. And Elm is probably the simplest functional language.

I don't have much time and I don't think I'll use Elm in any way. It would be great if there are some resources explain the ideas/concepts/patterns behind Elm, if you know any please share them.

* I've tried to rewrite `Radio` component to standard Elm arch - see https://github.com/MartinKavik/seed/tree/radio/examples/radio/src.

* `Config` is set by parent component/view - it contains business data derived from the parent's model or data based on context (e.g. hard-coded styles). `State` contains only internal non-business data. So the benefit is that there is a clear distinction between local state and global state + customizations. I don't know how much this is possible and wanted in your architecture.

one problem I can see with this approach, is that users cannot clone the radio with it's config using simple clone() on the state. they have to clone both the config and the state, so if a user want to use the same radio button on two different places they would end up cloning both state and config and pass them to each place they want, which is a lot of boilerplate thus making the code harder to read, with a few gain.

correct me if I'm mistaken.

@MuhannadAlrusayni
Copy link
Contributor Author

I have added counter example for now, other examples will be added after I add support for events for the elements/components.

@MartinKavik
Copy link
Member

MartinKavik commented Jan 20, 2020

one problem I can see with this approach, is that users cannot clone the radio with it's config using simple clone() on the state. they have to clone both the config and the state

The amount of boilerplate depends on your architecture and how much safety you want, etc. They are just trade-offs.

In the Elm arch. - Config and State is separated because it helps to prevent errors like the one where your two radios are checked in the same group.
Once we will be able to write Web Components (custom elements) in Seed, Config will represent a custom element attributes and State will represent element's private properties.

When the checked property is in State, you are on your own and you have to keep in mind, that only one radio should be checked (especially if you cloned some radios and don't know their previous checked value)

So again - they are trade-offs and it depends on you preferences and use-cases.


Ad counter example -

  • Can you remove #[derive(Clone)] from Msg? You should avoid it if possible, because once you have to send something that doesn't implement Clone, you have to use some wrappers like Rc.
  • Why .add(|item| { contains item?
  • Can we hide ev call as an implementation detail? E.g. btn.add_listener(Ev::Click, |_| Msg::Decrement)) (update: probably not due to helpers like input_ev).
  • Just tip - you can change .add_listener to .add_event_handler because you don't create real javascript listeners but only callbacks which are invoked by listeners. It will be also more consistent with Seed's window_events method (fn window_events(_: &Model) -> Vec<EventHandler<Msg>>).
  • Why are Button labels defined in Default, but listeners in view?
  • Are we able (and want (?)) to change API a bit so we have to choose msg mapper only once for each component? Example:
impl Default for MyApp {
    fn default() -> Self {
        Self {
            inc_btn: el::Button::with_label("+", Msg::IncBtn)
                   .add_event_handler(Ev::Click, |_| Msg::Increment),
   ...

impl Model<Msg, ()> for MyApp {
    fn update(&mut self, msg: Msg, orders: &mut impl Orders<Msg, ()>) {
        match msg {
            Msg::IncBtn(msg) => self.inc_btn.update(msg, &mut orders),
   ...

.add(self.inc_btn.render(theme))

@MuhannadAlrusayni
Copy link
Contributor Author

MuhannadAlrusayni commented Jan 20, 2020

When the checked property is in State, you are on your own and you have to keep in mind, that only one radio should be checked

Valid point, it make sense not to clone the check value.. hmm didn't think of that situation yet!. I will play with the Config pattern and see what I get.

Can you remove #[derive(Clone)] from Msg? You should avoid it if possible, because once you have to send something that doesn't implement Clone, you have to use some wrappers like Rc.

Sure, that was not possible before e900221 I believe.

Why .add(|item| { contains item ?

this is flexbox::Item which is the children for Flexbox, these provide functions that controls how the content will be displayed (.e.g grow(2.), center() ..etc)

  • Can we hide ev call as an implementation detail? E.g. btn.add_listener(Ev::Click, |_| Msg::Decrement)) (update: probably not due to helpers like input_ev).
  • Just tip - you can change .add_listener to .add_event_handler because you don't create real javascript listeners but only callbacks which are invoked by listeners. It will be also more consistent with Seed's window_events method (fn window_events(_: &Model) -> Vec<EventHandler>).

Sure, this is what I'm trying to do right now, it would be something like this:

el::Button::with_label("+", Msg::IncBtn)
    .on_click(|_| Msg::Increment)

Why are Button labels defined in Default, but listeners in view?

listeners in the view because I didn't add map_msg functionality yet, so I need to convert button to Node<btn::Msg> then map btn::Msg to the parent message than I can add listeners, this won't be the case after add map_msg functionality.

Are we able (and want (?)) to change API a bit so we have to choose msg mapper only once for each component?

I'm working on this, but this involve generics which make the code hard to work with :D.

thank you for your feedback and guidance.

@MuhannadAlrusayni
Copy link
Contributor Author

I have add map_msg functionality in map_msg branch, can you review it.

@MartinKavik
Copy link
Member

Can we define the associated Msg in one place?
Something like:

inc_btn: el::Button::with_label("+", Msg::IncBtn).events(|e| e.click(|_| Msg::Increment)),
...
Msg::IncBtn(msg) => self.inc_btn.update(msg, &mut orders),
...
.add(|item| item.content(vec![self.inc_btn.render(theme)]))

@MuhannadAlrusayni
Copy link
Contributor Author

I have tried to do it using generics Button<ParentMsg, MapMsg: FnOnce(Msg) -> ParentMsg + Clone + 'static with no success, because I the button cannot be stored inside types struct, enum anymore, that happens because when we want to store the button inside other types we have to define the type MapMsg which is closure that we cannot define explicitly:

struct App {
    home_btn: Button<AppMsg, ???>,
}

we can store the MapMsg using Rc something like:

pub struct Button<ParentMsg> {
    map_msg: Rc<dyn FnOnce(Msg) -> ParentMsg + 'static>
    ...
}

but I'm trying to minimize the use of trait object for performance, do think using Rc is fine in this case ?

@MartinKavik
Copy link
Member

There are many Rc,RefCell and Cell in Seed - user's comfort is the priority. If it helps with the public API or make the code more readable, use it. Rust+WASM is fast enough

@MuhannadAlrusayni
Copy link
Contributor Author

Hmm, I try to implement it using Rc but I got blocker, when I store map_msg in Button like this:

pub struct Button<ParentMsg> {
    map_msg: Rc<dyn FnOnce(Msg) -> ParentMsg + 'static>
    ...
}

and later when I want to render the button I have to call MessageMapper::map_msg to map the button msg to ParentMsg using the stored map_msg:

fn render(&self, theme: &impl Theme) -> Self::View {
    button![
        ev(Ev::Click, |_| Msg::Click)
    ]
    // calling MessageMapper::map_msg with the stored `map_msg`
    .map_msg(*Rc::clone(&self.map_msg))
}

I got error saying the trait 'std::clone::Clone' is not implemented for 'dyn FnOnce(btn::Msg) -> ParentMsg' to solve this problem we have to use wrapper type such Callback(Rc<...>) instead of plain Rc<...> and use that wrapper all around Seed and for sure implement From<T: FnOnce(Msg) -> OtherMs + Clone + 'static, OtherMs> for Callback I did this before in 8785844 but was removed later with the new refactor.

@MartinKavik
Copy link
Member

You have to use a "magic trick" - transform FnOnce + Clone to Fn - see https://github.com/MartinKavik/seed/tree/elm_component/examples/counter/src

@MuhannadAlrusayni
Copy link
Contributor Author

hehehe didn't think about this trick :D, I have updated the example with the new changes.

@MuhannadAlrusayni
Copy link
Contributor Author

will close this for now. if there is any update I might open it again.

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

2 participants