Skip to content
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

Jump to bottom

Feat: Add yearShape property to DatePickerThemeData #163909

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feat: Add yearShape property to DatePickerThemeData #163909

wants to merge 2 commits into from
+160 −18

Conversation

rkishan516
Copy link
Contributor

@rkishan516 rkishan516 commented Feb 22, 2025 •
edited
Loading

Feat: Add yearShape property to DatePickerThemeData
fixes: #163340

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].
  • I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
  • All existing and new tests are passing.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Feb 22, 2025
@rkishan516 rkishan516 force-pushed the date-picker-year-shape branch from 511d91d to fd0d544 Compare February 22, 2025 04:15
@dkwingsmt dkwingsmt self-requested a review March 12, 2025 18:50
@rkishan516 rkishan516 force-pushed the date-picker-year-shape branch from de44e9b to 793d8eb Compare March 13, 2025 01:33
@rkishan516 rkishan516 requested a review from matanlurey as a code owner March 13, 2025 01:33
dkwingsmt
dkwingsmt reviewed Mar 18, 2025
View reviewed changes
packages/flutter/lib/src/material/date_picker_theme.dart Outdated
@@ -944,6 +975,9 @@ class _DatePickerDefaultsM2 extends DatePickerThemeData {
elevation: 24.0,
shape: const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(4.0))),
dayShape: const MaterialStatePropertyAll<OutlinedBorder>(CircleBorder()),
yearShape: const MaterialStatePropertyAll<OutlinedBorder>(
RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(28.0))),
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that this probably changes the default shape. The default shape is always a pill shape, which has borderRadius equal to half the height. Using a fixed border radius here should result in a different shape if textScaleFactor is customized.

I think StadiumBorder should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, previously borderRadius was equal to half the height. So, I will use StadiumBorder instead.

packages/flutter/lib/src/material/date_picker_theme.dart Outdated
@@ -944,6 +975,9 @@ class _DatePickerDefaultsM2 extends DatePickerThemeData {
elevation: 24.0,
shape: const RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(4.0))),
dayShape: const MaterialStatePropertyAll<OutlinedBorder>(CircleBorder()),
yearShape: const MaterialStatePropertyAll<OutlinedBorder>(
RoundedRectangleBorder(borderRadius: BorderRadius.all(Radius.circular(28.0))),
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Also if the comment above is true, can you add a test case to verify that the default year shape is kept with a different textScaleFactor (likely by verifying the drawRRect operation)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkwingsmt I have pushed the change. Since i have made it StadiumBorder. It's respecting border radius based on that. Also as you noted, I have written test for that.

@rkishan516 rkishan516 force-pushed the date-picker-year-shape branch 5 times, most recently from 551cd89 to 1fd5613 Compare March 18, 2025 10:10
dkwingsmt
dkwingsmt reviewed Mar 18, 2025
View reviewed changes
dev/tools/gen_defaults/data/date_picker_modal.json Outdated
@@ -67,7 +67,7 @@
"md.comp.date-picker.modal.year-selection.year.selected.label-text.color": "onPrimary",
"md.comp.date-picker.modal.year-selection.year.selected.pressed.state-layer.color": "onPrimary",
"md.comp.date-picker.modal.year-selection.year.state-layer.height": 36.0,
"md.comp.date-picker.modal.year-selection.year.state-layer.shape": "md.sys.shape.corner.full",
"md.comp.date-picker.modal.year-selection.year.state-layer.shape": "md.sys.shape.corner.extra-large",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not, removing now.

dkwingsmt
dkwingsmt reviewed Mar 18, 2025
View reviewed changes
packages/flutter/test/material/date_picker_theme_test.dart
@@ -1206,4 +1205,67 @@ void main() {
);
}
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add a test to verify that assigning yearShape works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@dkwingsmt
Copy link
Contributor

Also analyzer error:

║ date_picker_theme.dart is not up-to-date with the token template file.
║ See: https://github.com/flutter/flutter/blob/main/dev/tools/gen_defaults to update the token template files.

@rkishan516 rkishan516 force-pushed the date-picker-year-shape branch 5 times, most recently from aaea387 to 7f31721 Compare March 19, 2025 11:23
@rkishan516 rkishan516 force-pushed the date-picker-year-shape branch from 7f31721 to 911ef06 Compare March 19, 2025 11:37
@rkishan516 rkishan516 requested a review from dkwingsmt March 19, 2025 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matanlurey matanlurey Awaiting requested review from matanlurey matanlurey is a code owner

@dkwingsmt dkwingsmt Awaiting requested review from dkwingsmt

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add yearShape property to DatePickerThemeData
2 participants
@rkishan516 @dkwingsmt