Conversation
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
I think this is a good API change. I had a few comments about the details.
|
|
||
| // Called by _CupertinoBackGestureDetector when a pop ("back") drag start | ||
| // gesture is detected. The returned controller handles all of the subsquent | ||
| // gesture is detected. The returned controller handles all of the subsequent |
| this.createRectTween, | ||
| this.flightShuttleBuilder, | ||
| this.placeholderBuilder, | ||
| this.transitionForUserGestures = false, |
There was a problem hiding this comment.
This is just bike shedding.
We call methods or callbacks that are triggered by "foo" onFoo. Since we're talking about a transition that's triggered by a user gesture here, maybe transitionOnUserGesture.
| final TransitionBuilder placeholderBuilder; | ||
|
|
||
| /// Whether to perform the hero transition if the [PageRoute] transition was | ||
| /// triggered by a user gesture such as a back swipe on iOS. |
| /// left in place once the Hero shuttle has taken flight. | ||
| final TransitionBuilder placeholderBuilder; | ||
|
|
||
| /// Whether to perform the hero transition if the [PageRoute] transition was |
There was a problem hiding this comment.
It's not really just a transition, it's a pop transition, right?
There was a problem hiding this comment.
Yes. Though from this perspective (docs of Hero), the reader shouldn't really be concerned about the direction of the transition.
| /// triggered by a user gesture such as a back swipe on iOS. | ||
| /// | ||
| /// If [Hero]s with the same [tag] on both the from and the to routes have | ||
| /// [transitionForUserGestures] set to true, a back swipe gesture will |
There was a problem hiding this comment.
I think "back swipe" gesture is too specific, it's just whatever "user gesture" triggered the call to Navigator.didStartUserGesture(). It's definitely useful to tie this stuff back to the iOS back gesture, since that's the only current user case. We just don't want to imply that other use cases aren't possible.
There was a problem hiding this comment.
I agree. Though I'm not too sure what to put more specifically.
I think didStartUserGesture is a bit of an implementation detail (in terms of how the HeroController receives events but we can envision changing that implementation detail without having to change the 'API' in the future).
Since I can't think of a specific other use case for the trigger and it's API stable now, it's probably better to be very specific in the doc and later expand than trying to restrict the API in the future.
| } | ||
|
|
||
| // Find the matching pairs of heros in from and to and either start or a new | ||
| // Find the matching pairs of heroes in from and to and either start or a new |
| PageRoute<dynamic> to, | ||
| Animation<double> animation, | ||
| HeroFlightDirection flightType, | ||
| bool fromGesture, |
There was a problem hiding this comment.
same comment as before about this parameter name
| /// | ||
| /// If present, the route immediately below `route` is `previousRoute`. | ||
| /// Though the gesture may not necessarily conclude at `previousRoute` if | ||
| /// canceled. |
There was a problem hiding this comment.
canceled => it is canceled.
Perhaps it's obvious but it seems like it would be worth explaining how a pop might be canceled and what that means vis didStopUserGesture().
| void didReplace({ Route<dynamic> newRoute, Route<dynamic> oldRoute }) { } | ||
|
|
||
| /// The [Navigator]'s routes are being moved by a user gesture. | ||
| /// The [Navigator]'s route `route` is being moved by a user gesture. |
There was a problem hiding this comment.
We should explain who needs to call this and when. From the sound of it, it's just a notification.
The doc should link to and summarize the role of didStopUserGesture as well as Hero.transitionFromUserGestures.
There was a problem hiding this comment.
Cross referenced didStopUserGesture.
Though this is a more generic callback API than heroes. We should probably not reference users of the API from the API.
| _userGesturesInProgress += 1; | ||
| if (_userGesturesInProgress == 1) { | ||
| final Route<dynamic> route = _history.last; | ||
| final Route<dynamic> previousRoute = route.willHandlePopInternally |
There was a problem hiding this comment.
final Route<dynamic> previousRoute = route.willHandlePopInternally || history.length < 2
? null
: _history[_history.length - 2]
3 participants
Fixes #9446
Lets Heroes be initiated from a back swipe gesture.
Changes signature for NavigatorObserver.didStartUserGesture from
to
Measures and inserts hero flight into overlay on first frame.
Use in Cupertino nav bars.
Breaking signature announce: https://groups.google.com/forum/#!topic/flutter-dev/SBAlcp6W_3g