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.

Sign up for GitHub

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

Jump to bottom

code comments for Scheduler and ComponentLink #1274

Merged
merged 3 commits into from
May 31, 2020
Merged

code comments for Scheduler and ComponentLink #1274

merged 3 commits into from
May 31, 2020

Conversation

kellpossible
Copy link
Contributor

Description

Code comments to help clarify the ComponentLink interface, and help others understand how the Scheduler currently operates.

siku2
siku2 reviewed May 30, 2020
View reviewed changes
yew/src/html/scope.rs Outdated Show resolved Hide resolved
siku2
siku2 reviewed May 30, 2020
View reviewed changes
yew/src/scheduler.rs Outdated Show resolved Hide resolved
kellpossible
kellpossible commented May 30, 2020
View reviewed changes
yew/src/html/scope.rs Outdated
///
/// Please be aware that currently this method
/// immediately/synchronously schedules a call to the
/// [Component](Component) interface.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

chatting with @siku2 in discord, decided it might be a nice idea to add a reference to the yewtil::LinkFuture::send_future() which may be one solution to problems that users may encounter with using this method. Currently this would be blocked by #1233 because LinkFuture isn't released yet.

@jstarry jstarry mentioned this pull request May 31, 2020
Closed
3 tasks
jstarry
jstarry reviewed May 31, 2020
View reviewed changes
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

So happy that more people understand the inner workings of the scheduler, thanks for adding these 😄

I added a few more nits, but otherwise looks awesome!

yew/src/html/scope.rs Outdated Show resolved Hide resolved
yew/src/html/scope.rs Outdated Show resolved Hide resolved
yew/src/html/scope.rs Show resolved Hide resolved
@kellpossible
Copy link
Contributor Author

thanks @jstarry for the review help! enjoying getting stuck more into yew :)

@kellpossible
Copy link
Contributor Author

I think this is probably ready for merge, might like to do a squash though beforehand to take out some of those extra commits.

@jstarry jstarry merged commit f3c8453 into yewstack:master May 31, 2020
@jstarry
Copy link
Member

jstarry commented May 31, 2020

I usually squash and merge 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siku2 siku2 siku2 left review comments

@jstarry jstarry jstarry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kellpossible @jstarry @siku2