Skip to content

Conversation

TahaTesser
Copy link
Member

@TahaTesser TahaTesser commented Nov 5, 2021

Related #63693

This introduces a standalone full-size Cupertino Calendar Picker, this will become one of the picker styles in CupertinoDatePicker. This is designed to be as close as possible to the native full-size calendar picker. Initial Implementation matches Material's calendar picker.

This calendar picker has two views grid and wheel (the wheel is customized to show month/year).

Light mode

Month/Day View Month/Year View

Dark mode

Month/Day View Month/Year View

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@flutter-dashboard flutter-dashboard bot added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Nov 5, 2021
@flutter-dashboard

This comment has been minimized.

@google-cla google-cla bot added the cla: yes label Nov 5, 2021
@TahaTesser TahaTesser requested a review from HansMuller November 5, 2021 14:50
@skia-gold

This comment has been minimized.

@skia-gold

This comment has been minimized.

@TahaTesser TahaTesser requested a review from jmagman November 5, 2021 15:16
@skia-gold

This comment has been minimized.

@skia-gold
Copy link

Gold has detected about 9 new digest(s) on patchset 5.
View them at https://flutter-gold.skia.org/cl/github/93130

@skia-gold
Copy link

Gold has detected about 18 new digest(s) on patchset 8.
View them at https://flutter-gold.skia.org/cl/github/93130

@skia-gold
Copy link

Gold has detected about 10 new digest(s) on patchset 9.
View them at https://flutter-gold.skia.org/cl/github/93130

@TahaTesser TahaTesser force-pushed the cupertino_calendar_picker branch from 890a389 to a2d0cee Compare November 8, 2021 14:08
@skia-gold
Copy link

Gold has detected about 6 new digest(s) on patchset 9.
View them at https://flutter-gold.skia.org/cl/github/93130

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #93130 at sha a2d0cee

@flutter-dashboard flutter-dashboard bot added the will affect goldens Changes to golden files label Nov 8, 2021
@TahaTesser
Copy link
Member Author

Updated flutter_localizations and update tests
This is ready to be reviewed :)

@HansMuller HansMuller removed their request for review December 3, 2021 17:17
@TahaTesser TahaTesser removed the request for review from jmagman February 10, 2022 18:42
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request! Left comments on a few minor things. I noticed the implementation of the new date picker is very similar to the material date picker. It will be great if we can take advantage of the existing material date picker implementation and avoid introducing duplicated code.

Discussed this with @HansMuller and @darrenaustin, they suggested moving the implementation to the material library and expose the calendar picker widget via a CalendarDatePicker .adaptive or DatePicker.adaptive constructor. What do you think?

/// The latest allowable [DateTime] that the user can select.
final DateTime? maximumDate;

/// Called when the user change a date in the picker.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Called when the user change a date in the picker.
/// Called when the user selects a date in the picker.

DateTime? initialDate,
this.onDisplayedMonthChanged,
}) : initialDate = initialDate ?? DateTime.now(),
minimumDate = DateTime(1900),
Copy link
Contributor

Choose a reason for hiding this comment

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

These 2 are not meant to be hard-coded I assume?

/// [maximumDate] must be after or equal to [minimumDate].
///
/// [initialDate] must be between [minimumDate] and [maximumDate] or equal to
/// one of them.
Copy link
Contributor

Choose a reason for hiding this comment

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

... and defaults to DateTime.now()?

/// to start in the year selection interface with [initialCalendarMode] set
/// to [CalendarPickerMode.year].
///
/// The [onDateChanged], and [initialCalendarMode] must be non-null.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The [onDateChanged] parameter

final CalendarPickerMode initialCalendarMode;

/// The earliest allowable [DateTime] that the user can select.
final DateTime initialDate;
Copy link
Contributor

Choose a reason for hiding this comment

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

documentation seems to be describing minimumDate

const Widget _centerSelectionOverlay = CupertinoPickerDefaultSelectionOverlay(capLeftEdge: false, capRightEdge: false);
const Widget _rightSelectionOverlay = CupertinoPickerDefaultSelectionOverlay(capLeftEdge: false);

const int _maxDayPickerRowCount = 6; // A 31 day month that starts on Saturday.
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm why is this 6 not 7? Also isn't this the same as DateTime.daysPerWeek? I noticed you are using DateTime.daysPerWeek somewhere else.


const int _maxDayPickerRowCount = 6; // A 31 day month that starts on Saturday.
// One extra row for the day-of-week header.
const double _dayPickerRowHeight = 47.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these have to be hard coded?

late CalendarPickerMode _mode;
late DateTime _currentDisplayedMonthDate;
late DateTime _selectedDate;
final GlobalKey _monthPickerKey = GlobalKey();
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these global keys needed?

__MonthPickerState createState() => __MonthPickerState();
}

class __MonthPickerState extends State<_MonthPicker> {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's double _ before the class name. Is that intended?

}

class __MonthPickerState extends State<_MonthPicker> {
final GlobalKey _pageViewKey = GlobalKey();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the global key here?

@guidezpl guidezpl removed the cla: yes label May 1, 2022
@TahaTesser
Copy link
Member Author

@LongCatIsLooong
Thanks so much for your valuable feedback.

Thank you for the pull request! Left comments on a few minor things. I noticed the implementation of the new date picker is very similar to the material date picker. It will be great if we can take advantage of the existing material date picker implementation and avoid introducing duplicated code.

Discussed this with @HansMuller and @darrenaustin, they suggested moving the implementation to the material library and expose the calendar picker widget via a CalendarDatePicker .adaptive or DatePicker.adaptive constructor. What do you think?

I'm gonna take this approach adaptive approach. I'll take your all suggestions here into count and work on this implementation in the new PR to avoid adding that code to this bubbled PR. Closing this for now.

@TahaTesser TahaTesser closed this Jun 3, 2022
@TahaTesser TahaTesser deleted the cupertino_calendar_picker branch June 3, 2022 12:51
@SimonVillage
Copy link
Contributor

@TahaTesser Do you have any timeline for this? Many people love this feature and are waiting for it! Thanks for your contribution.

@mehcode
Copy link

mehcode commented Jul 25, 2022

Any update here? I would really love having the iOS 14+ calendar picker in flutter.

@GianlucaAntolini
Copy link

Updates?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. will affect goldens Changes to golden files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants