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

fix: Update time picker dialog input size #163184

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

fix: Update time picker dialog input size #163184

wants to merge 3 commits into from
+55 −10

Conversation

rkishan516
Copy link
Contributor

fix: Update time picker dialog input size
Fixes: #162796

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 13, 2025
@rkishan516 rkishan516 force-pushed the timer-picker-overflow branch from 75b9ab3 to f8f9805 Compare February 13, 2025 03:39
MitchellGoodwin
MitchellGoodwin approved these changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for making this PR. My main concern was accessibility, and the text being scaled up. But there are clamping on the text scaling and specifically in the input mode we have room to increase the height a little, so this should be fine. Due to box constraints, this only affects the max size and will not affect input dialogs that do not need the extra space.

justinmc
justinmc approved these changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM assuming Google tests pass 👍 . I didn't realized this height would be hardcoded but I guess so.

@MitchellGoodwin
Copy link
Contributor

LGTM assuming Google tests pass 👍 . I didn't realized this height would be hardcoded but I guess so.

The material date pickers have hard coded maxes due to containing text scaling. The calendar view in particular has to watch out for a lot of text being scaled up and filling the screen. There's wiggle room with the input picker though, which has less text.

@MitchellGoodwin
Copy link
Contributor

I was wrong, Google testing is showing that the pickers are expanding when they don't need to. It doesn't look great. I'm not sure what the best solution is.

@rkishan516
Copy link
Contributor Author

I was wrong, Google testing is showing that the pickers are expanding when they don't need to. It doesn't look great. I'm not sure what the best solution is.

I will see, what other solution I can come up with.

@rkishan516 rkishan516 force-pushed the timer-picker-overflow branch from 0fe00c1 to 64796ee Compare March 1, 2025 02:04
@rkishan516
Copy link
Contributor Author

rkishan516 commented Mar 1, 2025 •
edited
Loading

@MitchellGoodwin Few solutions
We can make actions horizontally or vertically scrollable.
or
We can change dialog size dynamically with content size.
(As you said text scaling will be an issue).

@MitchellGoodwin
Copy link
Contributor

@MitchellGoodwin Few solutions We can make actions horizontally or vertically scrollable. or We can change dialog size dynamically with content size. (As you said text scaling will be an issue).

Making them scrollable is nice because it will avoid errors, but would lead to bad UI, in my opinion. We could do that as a last resort.

Changing the dialog size dynamically would be preferable. For the input time picker there's a lot of room to grow. All the careful constraints are target towards the calendar date picker, which fills up much more space. I'd have to double check but I believe font size can only increase by 10%. When I looked at the code, dynamically increasing the size does seem difficult, however, with the expanded widgets involved.

@rkishan516 rkishan516 force-pushed the timer-picker-overflow branch 3 times, most recently from 88ef8b0 to 7e8fd27 Compare March 4, 2025 02:37
@rkishan516
Copy link
Contributor Author

@MitchellGoodwin Few solutions We can make actions horizontally or vertically scrollable. or We can change dialog size dynamically with content size. (As you said text scaling will be an issue).

Making them scrollable is nice because it will avoid errors, but would lead to bad UI, in my opinion. We could do that as a last resort.

Changing the dialog size dynamically would be preferable. For the input time picker there's a lot of room to grow. All the careful constraints are target towards the calendar date picker, which fills up much more space. I'd have to double check but I believe font size can only increase by 10%. When I looked at the code, dynamically increasing the size does seem difficult, however, with the expanded widgets involved.

Yaa, I have converted Expanded into Flexible and given a top padding for actions same as dialog's bottom padding, such that it looks symmetrical. And since dial mode needs some height, so I have added maxHeight constraints. So, its dynamic till a point.

MitchellGoodwin
MitchellGoodwin reviewed Mar 4, 2025
View reviewed changes
packages/flutter/lib/src/material/time_picker.dart Outdated
padding: EdgeInsetsDirectional.only(start: theme.useMaterial3 ? 0 : 4),
padding: EdgeInsetsDirectional.only(
start: theme.useMaterial3 ? 0 : 4,
top: (pickerTheme.padding ?? defaultTheme.padding).vertical / 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is to add a gap between the inputs and the actions, right? Both deriving this spacing from the top padding and having it inflexible feels odd.

What if we added another Flexible widget to act as the spacing? Then it could shrink if needed, but the form section would have priority on space.

And if we used the uncompressed height as a derivative of the theme padding, we should call that out in the time picker theme documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think adding Flexible for spacing will start taking entire allowed height and which is not what we want. I wanted spacing around actions symmertical. So, I will change documentation in picker theme.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a flexible spacing widget could be added if a sub Column was added as well, setting the priority of how widgets grow.

Some of the checks are failing for overflow errors, so I imagine that since this space is static and cannot shrink it is lowering the amount of open space for the rest of the widgets.

@rkishan516 rkishan516 force-pushed the timer-picker-overflow branch 3 times, most recently from 842dc68 to 9845c9a Compare March 5, 2025 01:32
@rkishan516 rkishan516 force-pushed the timer-picker-overflow branch from 30868ed to 1307b1e Compare March 12, 2025 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinmc justinmc justinmc approved these changes

@MitchellGoodwin MitchellGoodwin Awaiting requested review from MitchellGoodwin

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.

Material showTimePicker (input mode) actions overflow when using longer confirm and cancel text
3 participants
@rkishan516 @MitchellGoodwin @justinmc