fix: Update time picker dialog input size

[rkishan516]

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.

[MitchellGoodwin]

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]

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

[MitchellGoodwin]

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]

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]

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]

@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]

@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]

@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]

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.

[rkishan516]

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.

[MitchellGoodwin]

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.

[MitchellGoodwin]

Apologies for letting this sit, I needed to find time to look into it. It still looks like it's expanding a bit too much when it doesn't need to.

What if we added a minHeight to this with the original minimum height 216, then added a mainAxisAlignment of spaceBetween to the Column and removed the spacer and flex from the Flexible.

We want to: have a small space between the form and actions if possible. If the form expands, then we want the space to go away first until it's gone completely, then start growing the dialog box.

Theoretically this combo would work. What do you think?

[rkishan516]

Yup, I think this should work. Since we are doing spaceBetween, so first that will shrink the space and then it will change dialog height.

[rkishan516]

@MitchellGoodwin I have pushed the change, Just one change from your suggestion i.e. keeping it limited to input and inputOnly mode.

[MitchellGoodwin]

What's the benefit of using a Builder? To be able to do the _entryMode check below in less lines?

[rkishan516]

Yes, @MitchellGoodwin.

[MitchellGoodwin]

This should be the original value. So above we'd want:

static const Size _kTimePickerInputSize = Size(312, 252);
static const double _kTimePickerInputMinimumHeight = 216;

then here it would be:

constraints: BoxConstraints(
  minHeight: _kTimePickerInputMinimumHeight,
  maxHeight: allowedSize.height,
),

This keeps dialogs that do not need to expand to not be bigger than they were before.

[rkishan516]

Sorry, My bad I didn't checked value of _kTimePickerMinInputSize and thought it will be 216. I have pushed required change.

[MitchellGoodwin]

LGTM! Thank you for the follow up fixes.