-
Couldn't load subscription status.
- Fork 3.5k
[animations] Set FadeScaleTransitionConfiguration to configuration default value #136
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
[animations] Set FadeScaleTransitionConfiguration to configuration default value #136
Conversation
FadeScaleTransitionConfiguration to configuration default valueThere 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.
Hi, thank you for the PR! I have some comments about some of the changes, but the bigger task would be to write unit tests to ensure that the default configuration used is respected.
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.
Specify in the API documentation for showModal that FadeScaleTransitionConfiguration will be used by default if it not specified.
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.
You need to add new tests to packages/animations/test/modal.dart ensuring that configuration is FadeScaleTransitionConfiguration by default.
What I would do is call showModal without specifying a configuration, but making sure that the animations forwards and reverse are the ones for FadeScaleTransitionConfiguration. Another thing that would be good to test here is that the barrier properties for FadeScaleTransitionConfiguration are used as well.
You can use some of the existing tests in that test file and the tests from packages/animations/test/fade_scale_transition_test.dart to guide you.
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.
Thanks, I've added the tests: b158029
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.
I would not take this line out of the test, since we still want to explicitly test that when FadeScaleTransitionConfiguration is used, we have the expected behavior. For example, if the default configuration for showModal were to ever change, this test would break
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.
Okay, I'll revert this 👍
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.
Ditto
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.
Ditto
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.
Ditto
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.
Ditto
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.
I think we can just leave this here, but it's up to you if this becomes the default behavior
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.
Thanks, I think the sample code should be simple, so omitting default value is better 🤔
b0b5d92 to
a9430dc
Compare
a9430dc to
b158029
Compare
|
Thanks for the polite review. I've reflected the points you've made, so please review it again 🙏 |
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.
Sorry for the delay in my code review, I've been busy with some other work lately and had not had the chance to look at the animations library PRs.
The changes look good! Just a small nit on the API documentation :)
| /// modal route that will be displayed, such as the enter and exit | ||
| /// transitions, the duration of the transitions, and modal barrier | ||
| /// properties. | ||
| /// By default, `configuration` is [FadeScaleTransitionConfiguration]. |
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 usually specify the default at the end of the API doc. Could you shift this to be the last line? Thank you!
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.
Thanks for the review again 🙏
But, I don't understand what change is desired 🤔
I think current default position is same as like this:
packages/packages/animations/lib/src/modal.dart
Lines 157 to 167 in f5411da
| /// A configuration object containing the properties needed to implement a | |
| /// modal route. | |
| /// | |
| /// The `barrierDismissible` argument is used to determine whether this route | |
| /// can be dismissed by tapping the modal barrier. This argument defaults | |
| /// to true. If `barrierDismissible` is true, a non-null `barrierLabel` must be | |
| /// provided. | |
| /// | |
| /// The `barrierLabel` argument is the semantic label used for a dismissible | |
| /// barrier. This argument defaults to "Dismiss". | |
| abstract class ModalConfiguration { |
Could you suggest the change by using multi-line code suggestions?
27f7511 to
41ae76a
Compare
Co-Authored-By: Shi-Hao Hong <[email protected]>
41ae76a to
0bd1dcf
Compare
|
Thanks for the suggestion 🙏 |
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! Thank you for working on this!

showModalworks well withconfiguration: const FadeScaleTransitionConfiguration()for typical use cases.#124 made it possible to set
FadeScaleTransitionConfigurationtoconfigurationdefault value.With this change,
showModalcan be just as easy to handle as showDialog.