Prevent out of bound date picker selections #7773

xster · 2017-02-01T02:16:07Z

Show out of bound dates as disabled
Prevent navigation into out of bound months by swipe or chevrons
Some small renames for readability

xster · 2017-02-01T02:16:59Z

Soliciting some feedback before I write the tests

xster · 2017-02-01T02:20:14Z

Addresses #6501. Will generalize into a generic predicate mechanism #637 and fix the year selector on a different PR.

abarth · 2017-02-01T02:59:57Z

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

    assert(currentDate != null);
    assert(onChanged != null);
    assert(displayedMonth != null);
+    assert(lastDate.isAfter(firstDate));


Can they not be the same?

Ah, I kept the previous behaviour but makes sense. Changed.

abarth · 2017-02-01T03:00:13Z

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

      if (day < 1) {
        labels.add(new Container());
      } else {
+        DateTime dayToBuild = new DateTime(year, month, day);


abarth · 2017-02-01T03:01:09Z

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

+            behavior: HitTestBehavior.opaque,
+            onTap: () {
+              DateTime result = new DateTime(year, month, day);
+              onChanged(result);


You already have dayToBuild, might as well just return it instead of making a new copy.

abarth · 2017-02-01T03:06:40Z

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

+  }
+
+  /// True if the earliest allowable month is displayed.
+  bool get _onFirstMonth {


We usually use the "on" prefix for callback closures. Maybe _isDisplayingFirstMonth ?

Ah, makes sense

abarth · 2017-02-01T03:08:40Z

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

  void initState() {
    super.initState();
+    // Initially display the pre-selected date.
+    _currentDisplayedMonthDate = new DateTime(config.selectedDate.year, config.selectedDate.month);


You need to update this member in didUpdateConfig as well. When your parent builds a new widget for this location in the tree and this State object is re-used, the framework calls didUpdateConfig to let you know that your config property now points to the new widget.

Understood. Thanks.

abarth · 2017-02-01T03:10:12Z

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

+        new DateTime(config.lastDate.year, config.lastDate.month));
+  }
+
+  void _monthPageChanged(int monthPage) {


We usually name implementations of onFoo callbacks something like _handleFoo, so in this case _handleMonthPageChanged.

abarth · 2017-02-01T03:11:25Z

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

+      ScrollableState state = _dayPickerListKey.currentState;
+      double newPage = state.scrollOffset.round() + 1.0;
+      state?.scrollTo(newPage, duration: _kMonthScrollDuration);
+      _monthPageChanged(newPage.toInt());


There's no need to call _monthPageChanged here. The scrollTo call will eventually cause the callback to be called for you.

Ah, what's what I was thinking when I opened #7764 since it wasn't working. If it's meant to, I'll dig a bit into why it wasn't working.

If I read correctly, seems like onPageChanged is a PageableState concept and scrollTo is a ScrollableState concept. And the various stuff on the call back chains of scrollTo doesn't re-enter PageableState.

I can either use APIs on PageableState directly in date_picker, override scrollTo on the way in and await then snap to page and notify, add some handle to super's end scroll, the offset setter or something in PageableState, or just always callback on didUpdateConfig.

I suspect calling pageable state APIs directly is probably best. Overriding would imply page users would lose low level scrollable programmatic APIs to scroll and not snap.

Ok, used the PageableState's fling API. PTAL

abarth · 2017-02-01T03:11:36Z

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

+      ScrollableState state = _dayPickerListKey.currentState;
+      double newPage = state.scrollOffset.round() - 1.0;
+      state?.scrollTo(newPage, duration: _kMonthScrollDuration);
+      _monthPageChanged(newPage.toInt());


abarth · 2017-02-01T03:12:31Z

Definitely looks in the right track.

Hixie · 2017-02-01T03:57:54Z

Regarding onFoo/didFoo/handleFoo, there's a discussion in the style guide.

xster · 2017-02-01T21:16:11Z

Ok, adding tests

xster · 2017-02-02T01:26:39Z

packages/flutter/lib/src/widgets/pageable_list.dart

  Future<Null> _flingToAdjacentItem(double scrollVelocity) {
    final double newScrollOffset = snapScrollOffset(scrollOffset + scrollVelocity.sign)
-      .clamp(snapScrollOffset(scrollOffset - 0.5), snapScrollOffset(scrollOffset + 0.5));
+      .clamp(snapScrollOffset(scrollOffset - 0.50001), snapScrollOffset(scrollOffset + 0.5));


This was a bug. The fix is a bit cheesy but I hear you're going to delete all this so I didn't make it too fancy.

Make sure you have a test for this specific case (with the date picker), and we'll make sure we don't regress it. :-)

It's somewhere in date_picker_test.dart :)

Hixie · 2017-02-02T05:48:27Z

packages/flutter/test/material/date_picker_test.dart

+    });
+  });
+
+  testWidgets('Cannot selecte a month past last date', (WidgetTester tester) async {


nit: here and below - typo "selecte"

oops, thanks

abarth · 2017-02-02T06:58:50Z

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

-    state?.scrollTo(state.scrollOffset.round() + 1.0, duration: _kMonthScrollDuration);
+    if (!_isDisplayingLastMonth) {
+      PageableState<PageableLazyList> state = _dayPickerListKey.currentState;
+      state?.fling(1.0);


You've got the type parameter in the key, so you can write this as _dayPickerListKey.currentState?.fling(1.0) if you like.

abarth · 2017-02-02T06:58:56Z

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

-    ScrollableState state = _dayPickerListKey.currentState;
-    state?.scrollTo(state.scrollOffset.round() - 1.0, duration: _kMonthScrollDuration);
+    if (!_isDisplayingFirstMonth) {
+      PageableState<PageableLazyList> state = _dayPickerListKey.currentState;


abarth · 2017-02-02T07:00:48Z

abarth · 2017-02-02T07:00:53Z

Thanks!

xster · 2017-02-02T19:25:13Z

Thanks for the reviews!

xster added 3 commits January 31, 2017 16:21

Readability minor refactors and partial scroll disables

5014451

Fix month switch button and swipe edges

acfbf10

Add disabled looks to days and make them untappable

05901c9

googlebot added the cla: yes label Feb 1, 2017

xster requested a review from abarth February 1, 2017 02:16

abarth reviewed Feb 1, 2017

View reviewed changes

Address some review minor edits

14bd08d

xster added 3 commits February 1, 2017 13:19

Fix test breaking typo

594b2c7

Added date picker tests for disabled days

7a435b0

Use fling instead of scroll to get callback for free

bc464a9

xster commented Feb 2, 2017

View reviewed changes

Hixie reviewed Feb 2, 2017

View reviewed changes

abarth reviewed Feb 2, 2017

View reviewed changes

Small review fixes

81eb4fb

xster merged commit 959db13 into flutter:master Feb 2, 2017

xster deleted the date-picker branch February 2, 2017 19:30

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

Prevent out of bound date picker selections #7773

Prevent out of bound date picker selections #7773

Uh oh!

Conversation

xster commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xster commented Feb 1, 2017

Uh oh!

xster commented Feb 1, 2017

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!

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!

abarth commented Feb 1, 2017

Uh oh!

Hixie commented Feb 1, 2017

Uh oh!

xster commented Feb 1, 2017

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!

abarth commented Feb 2, 2017

Uh oh!

abarth commented Feb 2, 2017

Uh oh!

xster commented Feb 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

xster commented Feb 1, 2017 •

edited

Loading