Document the buildTransitions() method#9182
Conversation
There was a problem hiding this comment.
Can you rename all the overrides to be consistent?
There was a problem hiding this comment.
I think we'd usually ave a comma after child and put the ) on the next line.
There was a problem hiding this comment.
s/PageRouteBuilder/[PageRouteBuilder]/ to linkify it.
There was a problem hiding this comment.
Maybe we don't need this paragraph. A bit verbose.
There was a problem hiding this comment.
Oh, that's backwards than what I would have assumed.
There was a problem hiding this comment.
You assumed correctly, I've fixed that.
|
|
|
FYI @xster |
HansMuller
force-pushed
the
build_transitions_doc
branch
from
91bebc8 to
bf58ae3
Compare
There was a problem hiding this comment.
'the secondaryAnimation executed in reverse defines how the route below it'?
There was a problem hiding this comment.
That's correct. I've fixed it.
There was a problem hiding this comment.
Maybe we don't need this paragraph. A bit verbose.
There was a problem hiding this comment.
I think this paragraph is too much opinion on how users would use the API
There was a problem hiding this comment.
'this route's transition as the new top of the navigator stack'?
There was a problem hiding this comment.
I think it's important to note that this parameter is rarely used but I've left out my opnion as to why.
There was a problem hiding this comment.
This whole paragraph sounds like it's referring to something that's not 'self' but the API is still asking the user to build animated widgets associated with the same 'self' route.
Perhaps 'The animation for this route when it's the second topmost route on the stack. It runs from 0.0 to 1.0 as another route is pushed on top of this one and from 1.0 to 0.0 as another route above this one is popped.
This animation runs together with the [primaryAnimation] of the route above it'?
There was a problem hiding this comment.
I agree, the way this is written (likewise for the other animation parameter) could be confusing. I've rewritten it in the same terms as the introductory material.
|
Let's chat in person. I still like Adam's idea of splitting the API into 2 methods for clarity. |
|
re: #9182 (comment) p.s. there are more animations in cupertino/page.dart to swap incoming->primary and outgoing->secondary for consistency |
…uildTransitions and buildPage animation paramter name to primaryAnimation
HansMuller
force-pushed
the
build_transitions_doc
branch
from
e771174 to
a4267c0
Compare
| /// | ||
| /// ```dart | ||
| /// new PageRouteBuilder( | ||
| /// pageBuilder: (BuildContext context, _, __) { |
There was a problem hiding this comment.
an argument for not using _: when someone copies this code, they will wonder what those parameters are. It will be a pain for them to find out, because they'll have to first look at PageRouteBuilder, then look up pageBuilder from there, then look up the typedef, then study it. If we just type and name the parameters here, it will be self-documenting.
| /// BuildContext context, | ||
| /// Animation<double> animation, | ||
| /// Animation<double> secondaryAnimation, | ||
| /// Widget child) { |
|
|
6 participants
Clarify the roles of the two buildTransitions() animation parameters. I've renamed them primaryAnimation and secondaryAnimation to make it easier to write about them.
Fixes #9132