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

Components implementation #91

Merged
merged 43 commits into from
Jan 15, 2018
Merged

Components implementation #91

merged 43 commits into from
Jan 15, 2018

Conversation

therustmonk
Copy link
Member

This is the first draft of components implementation. It breaks backward-compatibility and nominated to 0.3 version.

For demonstration I created example with 1'000 components which work independently. I broke contexts and services (because isolated loop of App used for every component), but I'll repair them.

@rivertam
Copy link
Contributor

rivertam commented Jan 7, 2018

Am I mistaken or are props and state both represented by the component struct's properties? What happens if a prop changes both as state and as a prop (from the inside and from the outside)?

@machineloop
Copy link
Contributor

machineloop commented Jan 8, 2018

Thanks @deniskolodin, this is great! I also have some similar questions about props vs state, how they're treated within a component. Right now it seems they're one and same, wondering if it might we worth separating them into two models or separate structs, one internally managed per component, one externally provided, much like React. But perhaps I'm missing something that works better in the Elm model...

@therustmonk therustmonk mentioned this pull request Jan 8, 2018
8 tasks
@therustmonk
Copy link
Member Author

This PR still experimental and I will improve ergonomics of API. Also I want to add properties and events. Apparently it will not fields of the model, but I haven't chosen the implementation yet. I continue to experiment.

@machineloop
Copy link
Contributor

Thanks @deniskolodin, great to see your experimental work so far, let us know if there's anything we can do to help

@machineloop
Copy link
Contributor

I've been playing with your latest commits, thanks for your hard work! One thing I've noticed is when I render a component, it's getting wrapped in a <div></div> by default. This probably works in 90% of cases, but sometimes gets in the way of using semantic and/or properly formed html.

Here's an example: I'm using a component to generate table rows, and the div tags wrap each row, preventing the table from rendering correctly:

screen shot 2018-01-12 at 8 56 25 am

Thanks again for all the time you're putting into Yew, I'm excited to see the components API coming together!

var task = null;
var pool = [];
var routine = function() { };
var schedule_routine = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of having routine start as a noop, as opposed to defining it at the top?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just a tradition to pass no-use-before-define lint. Is it safe to take advantage of var behaviour when everything actually defines at top? Will it work in the future? I saw that let doesn't work this way. What do you think?

Choose a reason for hiding this comment

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

Having two functions that call each other can get a little hairy. Is there a reason that schedule_routine is a different function? My immediate thoughts on this are that you could just inline that function and avoid the two functions referencing each other. Alternatively, the schedule_routine function can take routine as an argument, if you want it to be a general purpose utility to avoid code reuse. Another options is to pull the if block that calls schedule_routine into another funciton that is put into the setTimeout, so it would be something like:

...
task = setTimeout(() => {
    routine();
    if (pool.length > 0) {
        schedule_routine();
    }
});
...

@therustmonk
Copy link
Member Author

@machineloop Thank you for the review! You are right, enclosing <div> is wrong thing. It's a temporary solution, because vdom node expected an element for rendering now. I plan to fix it later when I will implement fragments #36. I tend to merge this PR soon, because it's huge and becomes harder to update ) Also I've tested all features of it and them work well. A few things I'll fix in the next PRs:

  • Remove enclosing <div>
  • Add Render trait (to unify render function declaration)
  • Allow Component to have child nodes through macro (and put children to component's Scope that makes possible to have components composition)

@machineloop
Copy link
Contributor

Thanks @deniskolodin, great work, looking forward to the next steps after merge!

@therustmonk therustmonk force-pushed the components branch 6 times, most recently from 9bd4679 to aefe098 Compare January 15, 2018 17:54
@therustmonk
Copy link
Member Author

This PR is ready to merge. I've tested components and them work fine 🎉 Still need to fix some imperfections after merging (remove redundant enclosing <div>). But this PR is too large to implement it here.

@therustmonk therustmonk merged commit daed071 into master Jan 15, 2018
@therustmonk therustmonk deleted the components branch January 21, 2018 16:06
@therustmonk therustmonk mentioned this pull request Mar 15, 2018
teymour-aldridge referenced this pull request in teymour-aldridge/yew Jul 8, 2020
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