flutter / uxr

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

Add go_router_prototype snippets to routing usability scenario code #82

Closed johnpryan closed 2 years ago

johnpryan commented 2 years ago

This adds code snippets using thego_router_prototype package. This package is a prototype that implements the proposed changes to go_router described in docs.flutter.dev/go/go-router-improvements.

InMatrix commented 2 years ago

I checked the behaviors of the demos. Everything was fine except running into an exception after pressing the "Create a new Wishlist" in the dynamic linking demo. I was on Flutter 3.0.0 (stable) and ran the demo in Chrome.

Restarted application in 95ms.
══╡ EXCEPTION CAUGHT BY GESTURE ╞═══════════════════════════════════════════════════════════════════
The following message was thrown while handling a gesture:
No RouteState in scope!
When the exception was thrown, this was the stack:
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 251:49            throw_
packages/go_router_prototype/src/state.dart 128:67                                      of
packages/scenario_code/dynamic-linking/dynamic_linking_go_router_prototype.dart 40:28   onCreate
packages/scenario_code/dynamic-linking/dynamic_linking_go_router_prototype.dart 116:25  <fn>
packages/flutter/src/material/ink_well.dart 1005:21                                     [_handleTap]
packages/flutter/src/gestures/recognizer.dart 198:24                                    invokeCallback
packages/flutter/src/gestures/tap.dart 613:48                                           handleTapUp
packages/flutter/src/gestures/tap.dart 298:5                                            [_checkUp]
packages/flutter/src/gestures/tap.dart 232:7                                            handlePrimaryPointer
packages/flutter/src/gestures/recognizer.dart 563:9                                     handleEvent
packages/flutter/src/gestures/pointer_router.dart 94:12                                 [_dispatch]
packages/flutter/src/gestures/pointer_router.dart 139:9                                 <fn>
dart-sdk/lib/_internal/js_dev_runtime/private/linked_hash_map.dart 21:13                forEach
packages/flutter/src/gestures/pointer_router.dart 137:17                                [_dispatchEventToRoutes]
packages/flutter/src/gestures/pointer_router.dart 123:7                                 route
packages/flutter/src/gestures/binding.dart 445:19                                       handleEvent
packages/flutter/src/gestures/binding.dart 425:14                                       dispatchEvent
packages/flutter/src/rendering/binding.dart 329:11                                      dispatchEvent
packages/flutter/src/gestures/binding.dart 380:7                                        [_handlePointerEventImmediately]
packages/flutter/src/gestures/binding.dart 344:5                                        handlePointerEvent
packages/flutter/src/gestures/binding.dart 302:7                                        [_flushPointerEventQueue]
packages/flutter/src/gestures/binding.dart 285:32                                       [_handlePointerDataPacket]
lib/_engine/engine/platform_dispatcher.dart 1105:13                                     invoke1
lib/_engine/engine/platform_dispatcher.dart 185:5                                       invokeOnPointerDataPacket
lib/_engine/engine/pointer_binding.dart 130:39                                          [_onPointerData]
lib/_engine/engine/pointer_binding.dart 543:18                                          <fn>
lib/_engine/engine/pointer_binding.dart 496:21                                          <fn>
lib/_engine/engine/pointer_binding.dart 210:16                                          loggedHandler
Handler: "onTap"
Recognizer:
  TapGestureRecognizer#340d5
════════════════════════════════════════════════════════════════════════════════════════════════════
johnpryan commented 2 years ago

Thanks @InMatrix, https://github.com/flutter/uxr/pull/82/commits/182593999dc45b26d88a22fa63494d7d76aa5165 fixes that bug. The RouteState.of() API needs to use the BuildContext of the StackedRoute's builder, not the BuildContext of WishlistApp.

InMatrix commented 2 years ago

I verified that the behavior of the demos matched the implementation using Router. However, I don't know enough about go_router to actually review the code. Could you request a review from an engineer on the team or someone who's familiar with go_router from the community? It's also fine to land it as is, since it's not for production.

johnpryan commented 2 years ago

@loic-sharma could you take a look?

loic-sharma commented 2 years ago

@johnpryan I did a first pass. Overall it looks good! I left a few nitpicks and a bunch of questions since I'm still ramping up. Feel free to ping me when you're ready for me to take another look!