flutter / uxr

UXR work for Flutter
BSD 3-Clause "New" or "Revised" License
227 stars 28 forks source link

[VRouter] adding 'Skipping stacks' and 'sign-in' scenarios #33

Closed lulupointu closed 3 years ago

lulupointu commented 3 years ago

Thanks for the suggestions about curly braces. I don't really understand why that look the way the do because they look way better on my IDE but never mind.

Regarding the vRouterKey, I understand your concern. My point of view was thank with BloC for example, you have to do such navigating inside the BloC since you cannot await for the event to end. Therefore I thought recommending to have the navigation closer to the state and further from the widgets might be a good idea. However I do understand your concern. Do you have any thought about what I just described? Should I recommend having the navigation in the widget tree as much as possible expect for special occasions (such as BloC authentication)? Or do you have a better idea ?

johnpryan commented 3 years ago

with BloC for example, you have to do such navigating inside the BloC since you cannot await for the event to end

I haven't used BloC very much, but I personally wouldn't recommend coupling the business logic (AppState) with the navigation logic.

I see that BloC developers recommend a similar pattern - putting a NavigatorKey in the BloC when using the Navigator API. This does feel like code smell to me. Either way, since this is not a BloC sample I don't see the benefit of doing that here.

recommending to have the navigation closer to the state and further from the widgets might be a good idea.

In my experience, it's better to decouple the UI code from the business logic. If I were to move the AppState class to a separate library, any unit tests that test AppState's behavior need to set the vRouterKey and mock it's behavior

Should I recommend having the navigation in the widget tree as much as possible expect for special occasions (such as BloC authentication)?

I think it's good practice to keep classes from the widgets library like GlobalKey out of the user's app state. I'm also curious why the user need to create a GlobalKey? Could they access VRouterState using something like VRouterState.of(context)?

This also makes me wonder, if AppState needs access to VRouterState, why not make VRouterState a required field instead? Why use GlobalKey<VRouterState>?

There might be other reasons to make the widget handle the navigation. Consider the case where another widget wants to use AppState to sign out but instead of redirecting, show a dialog?

The main goal here is to avoid participants in the study from being confused by something that's not related to the VRouter API - this is something that might trip people up as they read the code, and I think readers will understand VRouterState.of(context) in the the body of the build() since it's similar to how they access the NavigatorState today.

lulupointu commented 3 years ago

In my experience, it's better to decouple the UI code from the business logic. If I were to move the AppState class to a separate library, any unit tests that test AppState's behavior need to set the vRouterKey and mock it's behavior

Ok I understand, still not sure how to do it otherwise for event driven actions when using BloC but if this is a special case I'm ok with something more general.

I think it's good practice to keep classes from the widgets library like GlobalKey out of the user's app state. I'm also curious why the user need to create a GlobalKey? Could they access VRouterState using something like VRouterState.of(context)?

The GlobalKey was needed because of the navigation outside of the widget tree. In the widget tree, users can use VRouter.of(context) of context.vRouter, as you can see in e8f6c0d.

johnpryan commented 3 years ago

@lulupointu could you fix the README conflicts too? Sorry about that...

lulupointu commented 3 years ago

@lulupointu could you fix the README conflicts too? Sorry about that...

Done.