Skip to content

[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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 23, 2020
+195 −3
Merged

[animations] Set FadeScaleTransitionConfiguration to configuration default value #136

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bb3a9fb
Set showModal configuration default FadeScaleTransitionConfiguration
mono0926 Apr 11, 2020
4c9f4d2
Add documentation about `configuration` default value.
mono0926 Apr 15, 2020
b158029
Add test for modal with default configuration(FadeScaleTransitionConf…
mono0926 Apr 15, 2020
0bd1dcf
Shift `default` sentence to be the last line
mono0926 Apr 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/animations/example/lib/fade_scale_transition.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ class _FadeScaleTransitionDemoState extends State<FadeScaleTransitionDemo>
onPressed: () {
showModal<void>(
context: context,
configuration: const FadeScaleTransitionConfiguration(),
Copy link

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

Copy link
Contributor Author

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 🤔

builder: (BuildContext context) {
return _ExampleAlertDialog();
},
Expand Down
5 changes: 4 additions & 1 deletion packages/animations/lib/src/modal.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

import 'package:flutter/material.dart';

import 'fade_scale_transition.dart';

/// Signature for a function that creates a widget that builds a
/// transition.
///
Expand All @@ -27,6 +29,7 @@ typedef _ModalTransitionBuilder = Widget Function(
/// 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].
Copy link

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shihaohong

Thanks for the review again 🙏

But, I don't understand what change is desired 🤔

I think current default position is same as like this:

image

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?

///
/// The `useRootNavigator` argument is used to determine whether to push the
/// modal to the [Navigator] furthest from or nearest to the given `context`.
Expand All @@ -45,7 +48,7 @@ typedef _ModalTransitionBuilder = Widget Function(
/// the modal's characteristics.
Copy link

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.

Copy link

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.

Copy link
Contributor Author

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

Future<T> showModal<T>({
@required BuildContext context,
@required ModalConfiguration configuration,
ModalConfiguration configuration = const FadeScaleTransitionConfiguration(),
bool useRootNavigator = true,
WidgetBuilder builder,
}) {
Expand Down
190 changes: 190 additions & 0 deletions packages/animations/test/modal_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,185 @@ void main() {
},
);

testWidgets(
'showModal builds a new route with specified barrier properties '
'with default configuration(FadeScaleTransitionConfiguration)',
(WidgetTester tester) async {
await tester.pumpWidget(
MaterialApp(
home: Scaffold(
body: Builder(builder: (BuildContext context) {
return Center(
child: RaisedButton(
onPressed: () {
showModal<void>(
context: context,
builder: (BuildContext context) {
return const _FlutterLogoModal();
},
);
},
child: const Icon(Icons.add),
),
);
}),
),
),
);
await tester.tap(find.byType(RaisedButton));
await tester.pumpAndSettle();

// New route containing _FlutterLogoModal is present.
expect(find.byType(_FlutterLogoModal), findsOneWidget);
final ModalBarrier topModalBarrier = tester.widget<ModalBarrier>(
find.byType(ModalBarrier).at(1),
);

// Verify new route's modal barrier properties are correct.
expect(topModalBarrier.color, Colors.black54);
expect(topModalBarrier.barrierSemanticsDismissible, true);
expect(topModalBarrier.semanticsLabel, 'Dismiss');
},
);

testWidgets(
'showModal forwards animation '
'with default configuration(FadeScaleTransitionConfiguration)',
(WidgetTester tester) async {
final GlobalKey key = GlobalKey();
await tester.pumpWidget(
MaterialApp(
home: Scaffold(
body: Builder(builder: (BuildContext context) {
return Center(
child: RaisedButton(
onPressed: () {
showModal<void>(
context: context,
builder: (BuildContext context) {
return _FlutterLogoModal(key: key);
},
);
},
child: const Icon(Icons.add),
),
);
}),
),
),
);

// Start forwards animation
await tester.tap(find.byType(RaisedButton));
await tester.pump();

// Opacity duration: First 30% of 150ms, linear transition
double topFadeTransitionOpacity = _getOpacity(key, tester);
double topScale = _getScale(key, tester);
expect(topFadeTransitionOpacity, 0.0);
expect(topScale, 0.80);

// 3/10 * 150ms = 45ms (total opacity animation duration)
// 1/2 * 45ms = ~23ms elapsed for halfway point of opacity
// animation
await tester.pump(const Duration(milliseconds: 23));
topFadeTransitionOpacity = _getOpacity(key, tester);
expect(topFadeTransitionOpacity, closeTo(0.5, 0.05));
topScale = _getScale(key, tester);
expect(topScale, greaterThan(0.80));
expect(topScale, lessThan(1.0));

// End of opacity animation.
await tester.pump(const Duration(milliseconds: 22));
topFadeTransitionOpacity = _getOpacity(key, tester);
expect(topFadeTransitionOpacity, 1.0);
topScale = _getScale(key, tester);
expect(topScale, greaterThan(0.80));
expect(topScale, lessThan(1.0));

// 100ms into the animation
await tester.pump(const Duration(milliseconds: 55));
topScale = _getScale(key, tester);
expect(topScale, greaterThan(0.80));
expect(topScale, lessThan(1.0));

// Get to the end of the animation
await tester.pump(const Duration(milliseconds: 50));
topScale = _getScale(key, tester);
expect(topScale, 1.0);

await tester.pump(const Duration(milliseconds: 1));
expect(find.byType(_FlutterLogoModal), findsOneWidget);
},
);

testWidgets(
'showModal reverse animation '
'with default configuration(FadeScaleTransitionConfiguration)',
(WidgetTester tester) async {
final GlobalKey key = GlobalKey();
await tester.pumpWidget(
MaterialApp(
home: Scaffold(
body: Builder(builder: (BuildContext context) {
return Center(
child: RaisedButton(
onPressed: () {
showModal<void>(
context: context,
builder: (BuildContext context) {
return _FlutterLogoModal(key: key);
},
);
},
child: const Icon(Icons.add),
),
);
}),
),
),
);

// Start forwards animation
await tester.tap(find.byType(RaisedButton));
await tester.pumpAndSettle();
expect(find.byType(_FlutterLogoModal), findsOneWidget);

// Tap on modal barrier to start reverse animation.
await tester.tapAt(Offset.zero);
await tester.pump();

// Opacity duration: Linear transition throughout 75ms
// No scale animations on exit transition.
double topFadeTransitionOpacity = _getOpacity(key, tester);
double topScale = _getScale(key, tester);
expect(topFadeTransitionOpacity, 1.0);
expect(topScale, 1.0);

await tester.pump(const Duration(milliseconds: 25));
topFadeTransitionOpacity = _getOpacity(key, tester);
topScale = _getScale(key, tester);
expect(topFadeTransitionOpacity, closeTo(0.66, 0.05));
expect(topScale, 1.0);

await tester.pump(const Duration(milliseconds: 25));
topFadeTransitionOpacity = _getOpacity(key, tester);
topScale = _getScale(key, tester);
expect(topFadeTransitionOpacity, closeTo(0.33, 0.05));
expect(topScale, 1.0);

// End of opacity animation
await tester.pump(const Duration(milliseconds: 25));
topFadeTransitionOpacity = _getOpacity(key, tester);
expect(topFadeTransitionOpacity, 0.0);
topScale = _getScale(key, tester);
expect(topScale, 1.0);

await tester.pump(const Duration(milliseconds: 1));
expect(find.byType(_FlutterLogoModal), findsNothing);
},
);

testWidgets(
'State is not lost when transitioning',
(WidgetTester tester) async {
Expand Down Expand Up @@ -290,6 +469,17 @@ double _getOpacity(GlobalKey key, WidgetTester tester) {
});
}

double _getScale(GlobalKey key, WidgetTester tester) {
final Finder finder = find.ancestor(
of: find.byKey(key),
matching: find.byType(ScaleTransition),
);
return tester.widgetList(finder).fold<double>(1.0, (double a, Widget widget) {
final ScaleTransition transition = widget;
return a * transition.scale.value;
});
}

class _FlutterLogoModal extends StatefulWidget {
const _FlutterLogoModal({
Key key,
Expand Down