-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[animations] Introduce layoutBuilder parameter to PageTransitionSwitcher #170
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
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
@goderbauer This would solve the problem in flutter/flutter#54585 if I'm wondering if this should just be |
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is also missing tests. Can you please add some?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert these formatting changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert that alignment is not null and mention that in the doc above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: its -> it's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// Defaults to Alignment.center | |
| /// Defaults to [Alignment.center]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear what "the original alignment" is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert formatting change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert formatting change
This widget was heavily inspired by the AnimatedSwitcher (https://master-api.flutter.dev/flutter/widgets/AnimatedSwitcher-class.html), which also uses center as the default. That widget solves this problem by allowing the user to completely configure the layout via a provided layoutBuilder (https://master-api.flutter.dev/flutter/widgets/AnimatedSwitcher/layoutBuilder.html). We should probably do the same for this widget as well to give users maximum flexibility in case they want to change other aspects of the Stack. |
|
Should I implement a layoutBuilder? |
|
@federunco that seems like a more flexible way to approach this problem, since it allows devs to have a more fine-tuned control of the layout of the outgoing/incoming widgets. |
|
fly-by comment: looks like the analyzer is unhappy. It may require the submission of #173 to fix that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments about the documentation. It also needs a test or two to ensure that the default layout is preserved, as well as another test to ensure that a modified layoutBuilder is properly used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since alignment had been removed:
| /// The [duration], [reverse], [transitionBuilder] and [alignment] parameters | |
| /// The [duration], [reverse], and [transitionBuilder] parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add some sample code here for one or two use cases for modifying the layoutBuilder here. Otherwise, developers may not know how to go to this parameter to resolve their problem.
Also, since this property's purpose might be less intuitive from just reading its name, it might be helpful to put a pointer to here from the constructor or class API docs as well. Something like: "If the layout of the transitioning child widgets are not what you are looking for, see layoutBuilder for suggestions on how to configure the layout of the incoming and outgoing child widgets."
|
Hey @federunco, any progress on this PR? |
|
I should update the PR next wednesday, i'm quite busy at the moment |
|
Sounds good, thanks for the update. |
|
Seems like the formatter is still unhappy with one of the dart files. |
shihaohong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation itself looks good, there are two main types of comments:
- Some clarifications for the API just so that our users aren't confused by how to use the new layoutBuilder parameter and the terminology surrounding it.
- Seeing if there's some way to keep the instantiating
ChildEntryprivate to the library, since we do not want our users getting the wrong idea on how to use it and trying to instantiateChildEntryin their applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ChildEntry should not really be used as a "public API", but is necessary because it needs to be exposed as an argument for layoutBuilder, I think at the very least we should document how this is used for and that users of the animations package shouldn't be using this class unless they were using it together with PageTransitionSwitcher.layoutBuilder. This is my attempt to clarify this a little bit:
| /// Internal representation of a child that, now or in the past, was set on the | |
| /// PageTransitionSwitcher.child field, but is now in the process of | |
| /// transitioning. | |
| /// An internal representation of a child widget subtree that, now or in the past, | |
| /// was set on the PageTransitionSwitcher.child field and is now in the process of | |
| /// transitioning. | |
| /// | |
| /// This class should not be used outside of the context of | |
| /// [PageTransitionSwitcher.layoutBuilder], which uses this class in a callback to | |
| /// customize the layout of the transitioning widgets. | |
| /// | |
| /// For more information, please see the documentation of | |
| /// [PageTransitionSwitcher.layoutBuilder]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc/ @goderbauer I've never tried this before, but would it be possible to make this constructor private while keeping the class itself public? That way we don't get users that attempt to create a ChildEntry outside of the context of the PageTransitionSwitcher widget, but they can still use it in the callback for layoutBuilder:
| ChildEntry({ | |
| ChildEntry._({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can do that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use verbiage similar to other api documentation for dispose
| /// Animation disposal method | |
| /// Release the resources used by this object. | |
| /// | |
| /// The object is no longer usable after this method is called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some grammatical suggestions. I also removed the usage of "active entries" since users will be more familiar with referring to the new and old child widgets based on prior documentation.
| /// PageTransitionSwitcher uses the property [layoutBuilder] to lay out all | |
| /// active entries. | |
| /// If the layout of the transitioning child widgets are not what you are looking | |
| /// for, see [PageTransitionSwitcher.layoutBuilder] for suggestions on how to | |
| /// configure the layout of the incoming and outgoing child widgets | |
| /// PageTransitionSwitcher uses the [layoutBuilder] property to lay out the | |
| /// old and new child widgets. By default, [defaultLayoutBuilder] is used. | |
| /// See the documentation for [layoutBuilder] for suggestions on how to | |
| /// configure the layout of the incoming and outgoing child widgets if | |
| /// [defaultLayoutBuilder] is not your desired layout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| /// a [Column], that sizes its height depending on the heights of active entries. | |
| /// a [Column] that sizes its height depending on the heights of active entries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should define "active entries" here, since users might not get what these refer to. The only reference they have without peeking into the implementation is the concept of an "old" and "new" child widget, so this concept will need to be clarified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// This is an [PageTransitionSwitcherTransitionBuilder] function. | |
| /// See [PageTransitionSwitcherTransitionBuilder] for more information on the function signature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// The layout builder used as the default value of [layoutBuilder]. | |
| /// The default layout builder for [PageTransitionSwitcher]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, doesn't the [Stack] size itself to match the largest child, not the previous child?
| /// The new child is placed in a [Stack] that sizes itself to match the | |
| /// largest of the child or a previous child. The children are centered on | |
| /// each other. | |
| /// This function is the default way for how the new and old child widgets are placed | |
| /// during the transition between the two widgets. The new child is placed in a | |
| /// [Stack] that sizes itself to match the largest of the child or a previous child. | |
| /// The children are centered on each other. |
|
I've pushed all the suggested changes, how about making ChildEntry constructor private, any news? |
|
@federunco I think we should make the constructor private. That way, nobody can accidentally abuse the |
shihaohong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo a doc comment nit.
@goderbauer could you take a look to make sure this looks good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // An internal representation of a child widget subtree that, now or in the past, | |
| /// An internal representation of a child widget subtree that, now or in the past, |
|
Added to my review queue. Sorry for the delay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// was set on the PageTransitionSwitcher.child field and is now in the process of | |
| /// was set on the [PageTransitionSwitcher.child] field and is now in the process of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment on a public member should nor reference a private member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_activeEntries -> activeEntries
Or why not just entries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the first "is".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The see also section should always come last.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a little confusing. The layoutBuilder puts all children in a column, not just the new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What effect is this example trying to achieve? It seems odd to just place them all in a column?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not just the new child, all children are placed in a stack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can do that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we hand the full ChildEntry to the layoutBuilder? It seems that handing them just the transition Widgets from the ChildEntries would be enough. The layout builder shouldn't have access to the animation controllers in ChildEntry. Those shouldn't be messed with.
Similarly to AnimatedSwitcher's layoutBuilder it probably also makes sense to hand over the currentEntry and all other entries in separate arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, _ChildEntry should stay private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, _ChildEntry should stay private.
|
I made all the suggested changes, I used most of the AnimatedSwitcher code (just deleted some of the assertions that didn't make sense when the switcher was set in reverse and tweaked the code a bit) but I didn't test it on a real device, all the unit test passed btw. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave the comment about how we don't want to expose some of these fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: leave this comma as is, https://en.wikipedia.org/wiki/Serial_comma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| Widget currentChild, List<Widget> previousChildren, bool reverse) { | |
| Widget currentChild, List<Widget> previousChildren, bool reverse,) { |
... and then format with flutter format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use { } around the body of the if and put it on a separate line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leave this as-as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use { } around body and put body on new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| void _markChildWidgetCacheAsDirty() => _outgoingWidgets = null; | |
| void _markChildWidgetCacheAsDirty() { | |
| _outgoingWidgets = null; | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not work: It will cause the order of widgets to change unexpectedly if _currentEntry was previously shown below all _outgoingEntries instead of on top of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the layout builder for this widget should just get a List of widgets without knowledge of the current entry, to avoid this problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the old activeEntries list, right?
|
Also: the analyzer check is unhappy. You may have to rebase this PR to the latest master. |
Workaround for flutter/flutter#54585
|
It seems to still be failing the analyzer check. Make sure to run |
shihaohong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still LGTM, with a nit on the sample code
Co-authored-by: Shi-Hao Hong <shihaohong@google.com>
goderbauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, this looks good now after the two comments are resolved.
| /// This class should not be used outside of the context of | ||
| /// [PageTransitionSwitcher.layoutBuilder], which uses this class in a callback to | ||
| /// customize the layout of the transitioning widgets. | ||
| /// | ||
| /// For more information, please see the documentation of | ||
| /// [PageTransitionSwitcher.layoutBuilder]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems outdated. It's not used as part of the layoutBuilder anymore?
| /// [defaultLayoutBuilder]. | ||
| /// | ||
| /// The following example shows a [layoutBuilder] that places all entries inside | ||
| /// a [Column] that sizes its height depending on the heights of active entries. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, examples should show good practices. I can't think of any good use cases where putting the children in a column is a good idea. Maybe show an example where a Stack with a different alignment is used (e.g. topleft.)
|
The latest changes look like they address @goderbauer 's concerns and he's on vacation, so I'll merge the PR for now. If he has any issues with it, we can always just revert it or fix forward. Thank you so much for working through this PR! |
Fixes flutter/flutter#54585 by introducing layoutBuilder, which allows
PageTransitionSwitcherto lay out its children in a custom manner.