fixes Show Week Day in CupertinoDatePicker with CupertinoDatePickerMo… by sikandernoori · Pull Request #120052 · flutter/flutter

sikandernoori · 2023-02-06T01:24:25Z

Show Week Day in CupertinoDatePicker with CupertinoDatePickerMode.date

fixes #120051

Sample

If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].

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.
All existing and new tests are passing.

…de.date flutter#120051

jmagman · 2023-02-07T18:03:52Z

03:20 +4829 ~1: /b/s/w/ir/x/w/flutter/packages/flutter/test/cupertino/date_picker_test.dart: date picker has expected day of week                                                                      
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: exactly one matching node in the widget tree
  Actual: _TextFinder:<zero widgets with text "9" (ignoring offstage widgets)>
   Which: means none were found but one was expected

When the exception was thrown, this was the stack:
#4      main.<anonymous closure> (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/cupertino/date_picker_test.dart:1687:5)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

This was caught by the test expectation on the following line:
  file:///b/s/w/ir/x/w/flutter/packages/flutter/test/cupertino/date_picker_test.dart line 1687
The test description was:
  date picker has expected day of week
════════════════════════════════════════════════════════════════════════════════════════════════════

03:20 +4829 ~1 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/cupertino/date_picker_test.dart: date picker has expected day of week [E]                                                               
  Test failed. See exception logs above.
  The test description was: date picker has expected day of week

sikandernoori · 2023-02-13T23:07:39Z

@jmagman test case fixed.

MitchellGoodwin · 2023-02-22T18:06:20Z

@sikandernoori I'll review once all the tests are passing. Some of the tests don't look like infastructure issues outside of this PR, but this one looks related:

The following assertion was thrown running a test:
The finder "zero widgets with text "26" (ignoring offstage widgets)" (used in a call to "drag()")
could not find any matching widgets.

When the exception was thrown, this was the stack:
#0      WidgetController._getElementPoint (package:flutter_test/src/controller.dart:1236:7)
#1      WidgetController.getCenter (package:flutter_test/src/controller.dart:1175:12)
#2      WidgetController.drag (package:flutter_test/src/controller.dart:860:7)
#3      main.<anonymous closure> (file:///Volumes/Work/s/w/ir/x/w/flutter/examples/api/test/cupertino/date_picker/cupertino_date_picker.0_test.dart:21:18)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)

The test description was:
  Can change date, time and dateTime using CupertinoDatePicker

sikandernoori · 2023-02-22T19:21:50Z

@MitchellGoodwin failling test fixed.

MitchellGoodwin · 2023-02-23T17:05:14Z

@sikandernoori the infrastructure issues were fixed. Could you please rebase on the latest master branch to pick up the fixes?

sikandernoori · 2023-02-23T17:08:08Z

@sikandernoori the infrastructure issues were fixed. Could you please rebase on the latest master branch to pick up the fixes?

@MitchellGoodwin Done

jmagman · 2023-02-23T18:42:05Z

@sikandernoori the infrastructure issues were fixed. Could you please rebase on the latest master branch to pick up the fixes?

FYI As of yesterday we turned on the ability for any flutter-hacker contributor to rebase with the "Update branch" button (probably most useful for infra issues like this one)

MitchellGoodwin

Thank you for submitting this change. I left some mostly nit comments.

MitchellGoodwin · 2023-02-24T17:22:53Z

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

-          final String dayOfMonth = localizations.datePickerDayOfMonth(i);
-          if (longestText.length < dayOfMonth.length) {
-            longestText = dayOfMonth;
+        final int lenght = showDayOfWeek ? DateTime.daysPerWeek : 2;


nit: length instead of lenght

MitchellGoodwin · 2023-02-24T18:43:15Z

packages/flutter/lib/src/cupertino/localizations.dart

  const DefaultCupertinoLocalizations();

-  static const List<String> _shortWeekdays = <String>[
+  ///Short version of days of week.


nit: need a space in front of Short

MitchellGoodwin · 2023-02-24T18:50:16Z

examples/api/lib/cupertino/date_picker/cupertino_date_picker.0.dart

                        initialDateTime: date,
                        mode: CupertinoDatePickerMode.date,
                        use24hFormat: true,
+                        // This shows daya of week alongside day of month


nit: "day of the week"

MitchellGoodwin · 2023-02-24T18:54:59Z

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

+            final String dayOfMonth = localizations.datePickerDayOfMonth(i, showDayOfWeek ? wd : null);
+            if (longestText.length < dayOfMonth.length) {
+              longestText = dayOfMonth;
+            }


Instead of doing a nested loop to check every possible option to find the longest string, I think it would be faster to find the longest day of the week, and the longest day of the month in two separate strings and add them together? It would be log(m) + log(n) rather than log(m*n).

refactored.

MitchellGoodwin · 2023-02-27T18:43:45Z

packages/flutter/lib/src/cupertino/localizations.dart

  /// Day of month that is shown in [CupertinoDatePicker] spinner corresponding
  /// to the given day index.
  ///
+  /// if weekDay is provided then it will also show Day of week alongside day


nit: format to "If weekDay is provided then it will also show weekday name alongside the numerical day."

MitchellGoodwin · 2023-02-27T18:44:41Z

@sikandernoori I've left one more nit comment for the documentation, but other than that, this looks good to me.

MitchellGoodwin

LGTM. Thank you for dealing with my nitpicking.

sikandernoori · 2023-02-27T23:35:50Z

LGTM. Thank you for dealing with my nitpicking.

Happy to participate in quality work.

auto-submit · 2023-02-27T23:37:48Z

auto label is removed for flutter/flutter, pr: 120052, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

auto-submit · 2023-02-27T23:37:50Z

auto label is removed for flutter/flutter, pr: 120052, due to Validations Fail.

MitchellGoodwin · 2023-02-27T23:40:38Z

Oops, need another review. Let me ask around.

sikandernoori · 2023-02-27T23:44:15Z

Oops, need another review. Let me ask around.

from bot description I believe 1 review is enough, you have to pass "approve" label first.

MitchellGoodwin · 2023-02-27T23:47:03Z

from bot description I believe 1 review is enough, you have to pass "approve" label first.

No, it's because the PR has the "First-time contributor" label. Those need two reviews as a part of a security concern.

sikandernoori · 2023-02-27T23:50:14Z

from bot description I believe 1 review is enough, you have to pass "approve" label first.

No, it's because the PR has the "First-time contributor" label. Those need two reviews as a part of a security concern.

okay.

jmagman · 2023-02-27T23:50:18Z

Members of flutter-hackers access control group require one other flutter-hacker to approve. If the author is not a member of the flutter-hackers group, then two approvers are needed.
https://github.com/flutter/flutter/wiki/Contributor-access

Piinks

Welcome @sikandernoori, thanks for contributing!

…PickerMo… (flutter/flutter#120052)

#120052 introduces the `showDayOfWeek` flag to `CupertinoDatePicker` for mode `CupertinoDatePickerMode.date`, but the default `en` locale from `DefaultCupertinoLocalizations` is always used for the day of the week: https://github.com/flutter/flutter/blob/5103d757436124a08e6a8024ebacbf0cf4b2bacf/packages/flutter_localizations/lib/src/cupertino_localizations.dart#L116-L119 This PR introduces a new `intl.DateFormat` `weekdayFormat` to replace the default with the abbreviated weekday for any supported locales. | Before | After | | --- | --- | | <img width="379" alt="Screenshot 2024-07-09 at 5 08 43â�¯PM" src="proxy.php?url=https://github.com/flutter/flutter/assets/77553258/d6899c6b-bd0a-4484-a6a8-3ef1512aeae1"> | <img width="379" alt="Screenshot 2024-07-09 at 5 08 11â�¯PM" src="proxy.php?url=https://github.com/flutter/flutter/assets/77553258/f320c634-80d1-4f3b-adfd-ed85a9dfc3f6"> | Fixes #141875

fixes Show Week Day in CupertinoDatePicker with CupertinoDatePickerMo…

ab2ed77

…de.date flutter#120051

test case fixed

a0d08cd

Skandar Munir Ahmed and others added 2 commits February 14, 2023 01:10

day and weekday seprated

d35fd21

Merge branch 'flutter:master' into master

7dd1b8e

fix test

74da337

Merge branch 'flutter:master' into master

de59c8b

MitchellGoodwin self-requested a review February 23, 2023 17:54

MitchellGoodwin reviewed Feb 24, 2023

View reviewed changes

Skandar Munir Ahmed and others added 3 commits February 25, 2023 01:29

performance improvement

1996ea5

fix typo

b6a3533

Merge branch 'master' into master

0a51295

MitchellGoodwin reviewed Feb 27, 2023

View reviewed changes

Skandar Munir Ahmed added 2 commits February 28, 2023 00:28

reword comments

ace0fea

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

910360c

MitchellGoodwin approved these changes Feb 27, 2023

View reviewed changes

MitchellGoodwin added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 27, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 27, 2023

Merge branch 'master' into master

282c332

Piinks approved these changes Feb 28, 2023

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 28, 2023

auto-submit bot merged commit 681b72c into flutter:master Feb 28, 2023

engine-flutter-autoroll mentioned this pull request Feb 28, 2023

Roll Flutter from c8d80163b03f to f032a4d64891 (17 revisions) flutter/packages#3320

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 28, 2023

681b72c fixes Show Week Day in CupertinoDatePicker with CupertinoDate…

5f6015b

…PickerMo… (flutter/flutter#120052)

HansMuller mentioned this pull request Mar 3, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

681b72c fixes Show Week Day in CupertinoDatePicker with CupertinoDate…

3b89c2c

…PickerMo… (flutter/flutter#120052)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 10, 2023

681b72c fixes Show Week Day in CupertinoDatePicker with CupertinoDate…

d4aa1dc

…PickerMo… (flutter/flutter#120052)

victorsanni mentioned this pull request Jul 9, 2024

Use correct locale for CupertinoDatePicker weekday #151494

Merged

9 tasks

Conversation

sikandernoori commented Feb 6, 2023

Sample

Pre-launch Checklist

Uh oh!

jmagman commented Feb 7, 2023

Uh oh!

sikandernoori commented Feb 13, 2023

Uh oh!

MitchellGoodwin commented Feb 22, 2023

Uh oh!

sikandernoori commented Feb 22, 2023

Uh oh!

MitchellGoodwin commented Feb 23, 2023

Uh oh!

sikandernoori commented Feb 23, 2023

Uh oh!

jmagman commented Feb 23, 2023

Uh oh!

MitchellGoodwin 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!

MitchellGoodwin commented Feb 27, 2023

Uh oh!

MitchellGoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

sikandernoori commented Feb 27, 2023

Uh oh!

auto-submit bot commented Feb 27, 2023

Uh oh!

auto-submit bot commented Feb 27, 2023

Uh oh!

MitchellGoodwin commented Feb 27, 2023

Uh oh!

sikandernoori commented Feb 27, 2023

Uh oh!

MitchellGoodwin commented Feb 27, 2023

Uh oh!

sikandernoori commented Feb 27, 2023

Uh oh!

jmagman commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmagman commented Feb 27, 2023 •

edited

Loading