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

WIP updates for preview 5 #73

Merged
merged 2 commits into from May 27, 2019

Conversation

Projects
None yet
3 participants
@rynowak
Copy link
Collaborator

commented May 12, 2019

I'm going through the workshop again and finding little things to polish as well as updating things that have changed.

Feel free to discuss or debate anything that I added, I tried to polish up some sections where I thought we introduced a new concept without explanation.

I reworked and simplified everything about the appstate pattern. Now that we have EventCallback it works pretty well in the simplified form and serves as a good explanation of why EventCallback is needed.

@rynowak rynowak requested review from SteveSandersonMS and danroth27 May 12, 2019

Show resolved Hide resolved docs/02-customize-a-pizza.md Outdated
Show resolved Hide resolved docs/02-customize-a-pizza.md Outdated

@rynowak rynowak force-pushed the rynowak/preview5-updates branch from 9e07482 to 43b910b May 12, 2019

Ryan Nowak
Updates for preview 5
Includes some cleanup and some additional explanations of concepts.

Includes a rework of the *appstate* section to be simpler. Now that we
have EventCallaback, the actual mechanics of using *appstate* are much
simpler.

@rynowak rynowak force-pushed the rynowak/preview5-updates branch from 43b910b to b8e98e2 May 13, 2019

@rynowak rynowak marked this pull request as ready for review May 13, 2019

@SteveSandersonMS

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

It definitely looks good to have so many fewer StateHasChanged calls!

About the simplifications to the OrderState class - while I agree they are good simplifications for this tutorial, would you still agree that in the more general case you'd keep the StateChanged event and manually trigger it from everything that mutates internal state? Otherwise we're relying on the idea that, on each state change, we just so happen to be in a place to trigger a refresh on the right component. If there were cases where component A causes a state change, and unrelated component B wants to observe it, we wouldn't be able to achieve that through EventCallback alone.

I'm still in support of your change, just want to synchronise our ideas about the broader appstate-like pattern.

@EdCharbeneau

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

@rynowak @danroth27 Will this be merged soon? I'm giving this workshop on 5/31.

@rynowak

This comment has been minimized.

Copy link
Collaborator Author

commented May 26, 2019

Yeah, I plan to do it this weekend 👍

@rynowak

This comment has been minimized.

Copy link
Collaborator Author

commented May 27, 2019

I'm still in support of your change, just want to synchronise our ideas about the broader appstate-like pattern.

I think this would be true if you wanted to use AppState as a global - ie to handle events from many levels of hierarchy. Maybe it warrants more discussion, but I simplified with use case in the workshop for AppState - which removes all of the ceremony.

So I guess there are two use cases:

  1. Refactor some logic for a complicated component into a class
  2. Refactor logic for the whole app into a class

The former is what's in the workshop now and it doesn't require StateHasChanged() nor does it require disposing and detatching. I thought this was the plan based on the feedback we got from the first workshop. If you feel strongly that we should teach option 2 in the workshop then I'd be happy to put it back to the way it was.

Ryan Nowak

@rynowak rynowak merged commit 2856ede into master May 27, 2019

1 check passed

license/cla All CLA requirements met.
Details

@rynowak rynowak deleted the rynowak/preview5-updates branch May 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.