Change color of chevrons when scrolling month date picker (#5267) #19195

mjohnsullivan · 2018-07-10T03:15:18Z

Chevrons in month date picker change to disabled color when scrolling starts, and back to the default icon color when scrolling ends.

Hixie · 2018-07-12T21:27:18Z

packages/flutter/lib/src/material/date_picker.dart

+                      ? null
+                      : '${localizations.previousMonthTooltip} ${localizations.formatMonthYear(_previousMonthDate)}',
+                  onPressed:
+                      _isDisplayingFirstMonth ? null : _handlePreviousMonth,


wrapping here doesn't seem to make this more readable... why not leave this on one line?

(the onPressed case, i mean)

Hixie · 2018-07-12T21:29:45Z

packages/flutter/test/material/date_picker_test.dart

+
+    // Initial chevron animation state should be dismissed
+    // An AlwaysStoppedAnimation is also found and is ignored
+    find.byType(FadeTransition).evaluate().forEach((Element element) {


looking for the fade transition seems a bit brittle... i recommend looking for the chevron icon's render object's parent, or some such.

How about this:

for(Element element in find.byType(IconButton).evaluate()) {
final RenderAnimatedOpacity ft = element.ancestorRenderObjectOfType(
const TypeMatcher());
expect(ft.opacity.value, equals(1.0));
expect(ft.opacity.status, equals(AnimationStatus.dismissed));
}

Hixie · 2018-07-12T21:30:06Z

packages/flutter/test/material/date_picker_test.dart

+
+    // Drag and hold the picker to test for the opacity change
+    final TestGesture gesture = await tester.startGesture(const Offset(100.0, 100.0));
+    await gesture.moveBy(const Offset(50.0, 100.0), );


trailing comma here is weird, did you mean to give another argument?

Typo on my side

…9277)" (#19295)" (#19322) This reverts commit cc122f1.

…19327) This reverts commit e22f997. Adding the exit(exitCode) fixes the hang. Tested by running `fx run-host-tests` with a topaz checkout. Verified the hang before, no hang after.

…19318)

HansMuller

This PR looks good. The test could be cleaned up a little.

HansMuller · 2018-07-13T18:04:04Z

packages/flutter/lib/src/material/date_picker.dart

+    _chevronOpacityController = new AnimationController(
+      duration: const Duration(milliseconds: 500), vsync: this
+    );
+    final curve = new CurvedAnimation(parent: _chevronOpacityController, curve: Curves.easeInOut);


A type is needed here, Animation<double>, and maybe curve is the wrong name for the variable since it's not a Curve.

Sorry to nit pick. You could avoid the whole issue by just writing:

_chevronOpacityAnimation = new Tween(begin: 1.0, end: 0.5).animate( new CurvedAnimation( parent: _chevronOpacityController, curve: Curves.easeInOut, ), );

Nitpick away as it leads to cleaner code! Will revise to clean up.

HansMuller · 2018-07-13T18:07:57Z

packages/flutter/lib/src/material/date_picker.dart

-              itemBuilder: _buildItems,
-              onPageChanged: _handleMonthPageChanged,
+            child: new NotificationListener<ScrollStartNotification>(
+              onNotification: (_) => _handleScrollStartNotification(context),


You don't need to pass the context along here, since it's a State property. Rather than creating a closure, you could just make the _handleFoo methods NotificationListenerCallbacks

Will move the functions into inline closures as they're short and don't clutter the build() code too much.

HansMuller · 2018-07-13T18:10:57Z

packages/flutter/lib/src/material/date_picker.dart

+                opacity: _chevronOpacityAnimation,
+                child: new IconButton(
+                  icon: new Icon(Icons.chevron_left),
+                  tooltip: _isDisplayingFirstMonth


More nit pickery. The way this was formatted helped highlight the fact that tooltip and onPressed values had similar structure. If you're going to do this, then 2-space indent.

Auto-formatter strikes; will revert to previous formatting

HansMuller · 2018-07-13T18:11:48Z

packages/flutter/lib/src/material/date_picker.dart

+                opacity: _chevronOpacityAnimation,
+                child: new IconButton(
+                  icon: new Icon(Icons.chevron_right),
+                  tooltip: _isDisplayingLastMonth


same comment as earlier.

HansMuller · 2018-07-13T18:12:26Z

packages/flutter/test/material/date_picker_test.dart

+      expect(ft.opacity.status, equals(AnimationStatus.dismissed));
+    }
+
+


Lots of extra blank lines in this test

HansMuller · 2018-07-13T18:15:54Z

packages/flutter/test/material/date_picker_test.dart

+    );
+
+
+    for(Element element in find.byType(IconButton).evaluate()) {


https://docs.flutter.io/flutter/flutter_test/CommonFinders/descendant.html

You can define a finder value once rather than using this block of code over and over.

Switching to:

final Finder chevronFinder = find.byType(IconButton);
chevronFinder.precache();

HansMuller · 2018-07-13T23:42:28Z

NICE. LGTM

googlebot · 2018-07-14T00:29:25Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

mjohnsullivan · 2018-07-14T00:36:29Z

My copy of the git repo is hopelessly messed up. Will pull a fresh copy and create a new PR

Matt Sullivan and others added 30 commits November 13, 2017 09:19

Fixed typo in navigator.dart doc comments

bd69d5b

Merge branch 'master' of github.com:flutter/flutter

e08c54c

Merge branch 'master' of github.com:flutter/flutter

06c95a7

Added survey js to docs template

c893fd6

Merge branch 'master' of github.com:flutter/flutter

e89db43

Merge remote-tracking branch 'upstream/master'

0cfe349

Merge branch 'master' of github.com:flutter/flutter

9cd6a85

Fixed template for driver test file

cdef809

Merge branch 'master' of github.com:flutter/flutter

85b206f

Merge branch 'master' of github.com:mjohnsullivan/flutter

5b85bf5

Merge remote-tracking branch 'upstream/master'

81bd4fb

Added supported image formats to the Image class comments

20fbeb1

Merge remote-tracking branch 'upstream/master'

a12e368

Merge branch 'master' of github.com:mjohnsullivan/flutter

dd853e6

Merge remote-tracking branch 'upstream/master'

0d3123b

Merge remote-tracking branch 'upstream/master'

6122a4d

Merge remote-tracking branch 'upstream/master'

c03cfcb

Merge branch 'master' of github.com:mjohnsullivan/flutter

f056e6d

Fixed conflict on Image doc strings

be9367f

Merge remote-tracking branch 'upstream/master'

1235caa

Added sample code to AnimatedOpacity docs

524132a

Merge remote-tracking branch 'upstream/master'

bc46bd4

Merge remote-tracking branch 'upstream/master'

b2e32cf

Merge branch 'master' of github.com:mjohnsullivan/flutter

50f1d9f

Merge remote-tracking branch 'upstream/master'

24c7fb1

Merge remote-tracking branch 'upstream/master'

1c0d03a

Merge remote-tracking branch 'upstream/master'

d4c4658

Merge remote-tracking branch 'upstream/master'

1086081

Merge remote-tracking branch 'upstream/master'

40aa3ff

Change color of chevrons when scrolling month date picker. Fix for 5267

2775dbe

Hixie reviewed Jul 12, 2018

View reviewed changes

aam and others added 5 commits July 12, 2018 14:59

Revert "Revert "Roll engine to fed2ea4 (includes dart roll) again (#1…

7ebf272

…9277)" (#19295)" (#19322) This reverts commit cc122f1.

Tidied up formatting and rewrote test

d58b733

Roll engine to c5a63d (#19288)

579d2fb

Revert "Revert "Use runTests in fuchsia tester. (#19178)" (#19321)" (#…

35f3740

…19327) This reverts commit e22f997. Adding the exit(exitCode) fixes the hang. Tested by running `fx run-host-tests` with a topaz checkout. Verified the hang before, no hang after.

Correct contentPadding type in InputDecoration.debugFillProperties (#…

698bda6

…19318)

HansMuller reviewed Jul 13, 2018

View reviewed changes

Jonah Williams and others added 3 commits July 13, 2018 13:00

have text finder convert Text.rich to plain text for comparison (#19270)

8359d99

Minor tweaks to clean up code

6044e3d

Normalized unit test further

ea54119

xster and others added 10 commits July 13, 2018 16:46

Add HeroController to CupertinoApp (#19326)

e32e447

Added sample code to AnimatedOpacity docs

27da63c

Change color of chevrons when scrolling month date picker. Fix for 5267

2f4032f

Change color of chevrons when scrolling month date picker (#5267)

3e20f09

Change color of chevrons when scrolling month date picker (#5267)

d6de17e

Changed chevron color change to a FadeAnimation and added test

d1c672a

Tidied up formatting and rewrote test

3f37b26

Minor tweaks to clean up code

c4f06c9

Normalized unit test further

59c914a

Merge branch 'master' of github.com:mjohnsullivan/flutter

5dc8499

googlebot added cla: no and removed cla: yes labels Jul 14, 2018

mjohnsullivan closed this Jul 14, 2018

mjohnsullivan mentioned this pull request Jul 14, 2018

Chevrons in month picker are semi-transparent when the month is scrolled #19363

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2021

		expect(ft.opacity.status, equals(AnimationStatus.dismissed));
		}

		);


		for(Element element in find.byType(IconButton).evaluate()) {

Change color of chevrons when scrolling month date picker (#5267) #19195

Change color of chevrons when scrolling month date picker (#5267) #19195

Uh oh!

Conversation

mjohnsullivan commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HansMuller commented Jul 13, 2018

Uh oh!

googlebot commented Jul 14, 2018

Uh oh!

mjohnsullivan commented Jul 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants