Date picker layout exceptions by darrenaustin · Pull Request #31514 · flutter/flutter

darrenaustin · 2019-04-23T19:58:32Z

Description

This PR addresses several layout issues with the Material Date Picker when displaying on smaller sized devices or with an increased text scale factor. It removes several hard coded sizes that were causing the layouts to overflow. This change will lead to some awkward layouts with scrolling the month grid vertically, but it should no longer throw overflow exceptions. We will address smaller layouts more fully in a future update.

Example of layout fixes

Before/After on a Pixel with Large Text scale selected:

Before/After on a Pixel with a Chinese locale:

Before/After on a small display (320x521) with Large Text scale selected:

Related Issues

Fixes: #21383, #20171, #19744, #17745

Tests

I added the following tests:

A group of device configuration tests that will try to bring up the date picker in various sized screens, orientations and text scale factors.
A group of device configuration tests to the flutter_localizations tests to bring up the date picker in various locales that were having issues with layout.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

…t removed hard coded sizes to allow the grid view to scroll instead of overflowing.

…tuff.

HansMuller

This looks good, just some minor feedback.

It would be helpful if the issue's description included a few before/after screenshots that illustrated the layout changes.

HansMuller · 2019-04-26T17:58:39Z

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

            child: GridView.custom(
              gridDelegate: _kDayPickerGridDelegate,
              childrenDelegate: SliverChildListDelegate(labels, addRepaintBoundaries: false),
+              padding: EdgeInsets.zero,


If we set this to zero don't we risk having the bottom of the grid end up behind the display's notch?

I don't think so, as this GridView is embedded in a PageView.builder which puts the dialog header above it. If this padding isn't set to zero, by default there is an unneeded gap at the top of the grid, which will make it more likely that it will need to scroll on smaller screens.

HansMuller · 2019-04-26T18:01:39Z

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

  @override
  Widget build(BuildContext context) {
    return SizedBox(
-      width: _kMonthPickerPortraitWidth,


Looking at this through the github review microsope ... it seems odd to configure the height of the MonthPicker with _kMaxDayPickerHeight. If that's intentional, maybe a comment that explains why.

Yeah, this is a bit weird. The MonthPicker just wraps the DayPicker with horizontal navigation to switch months. I added a comment that hopefully makes this easier to follow.

HansMuller · 2019-04-26T18:13:17Z

packages/flutter/test/material/date_picker_test.dart

+
+void _screenConfigTests() {
+
+  const Size kPixel1Portrait = Size(1070, 1770);


A comment about these sizes, like what they're based on, why we chose them, would be helpful.

Just pushed a fix that renamed them and commented them with hopefully better descriptions.

HansMuller · 2019-04-26T18:14:11Z

packages/flutter/test/material/date_picker_test.dart

 }
+
+void _screenConfigTests() {
+


Please insert a comment (probably several) like this here:

// Regression test for https://github.com/flutter/flutter/issues/21383

Or, if it makes more sense, in the specific testWidgets tests.

HansMuller · 2019-04-26T18:17:18Z

packages/flutter/test/material/date_picker_test.dart

+  }
+
+  testWidgets('display on Pixel1 - portrait', (WidgetTester tester) async {
+    await _showPicker(tester, kPixel1Portrait);


These tests are verifying that we don't assert. Just to make that clear it would be good to append:

expect(tester.takeException(), isNull);

Nice suggestion. I have added this to all the tests.

HansMuller · 2019-04-26T18:20:40Z

packages/flutter_localizations/test/date_picker_test.dart

    await tester.tap(find.text('ANNULER'));
  });
+
+  group('screen configurations', () {


What these tests have in common seems to be that we're varying locale?

I renamed this group to "locale fonts don't overflow layout". Couldn't think of a better description. Let me know if you have one :).

HansMuller · 2019-04-26T18:21:36Z

packages/flutter_localizations/test/date_picker_test.dart

+
+    const Size kPixel1Portrait = Size(1070, 1770);
+    const Size kPixel1Landscape = Size(1770, 1070);
+    Future<void> _showPicker(WidgetTester tester, Locale locale, Size size) async {


add a blank line here?

HansMuller · 2019-04-26T18:22:08Z

packages/flutter_localizations/test/date_picker_test.dart

+
+  group('screen configurations', () {
+
+    const Size kPixel1Portrait = Size(1070, 1770);


As before, maybe there's something to say about the sizes

- Renamed the sizes used for testing and documented them with comments. - Added more comments around the tests to better describe them and reference the bugs they test for.

…scape picker always try to take up 2/3 of the space and give the other 3rd to the header.

HansMuller

LGTM

darrenaustin added 3 commits April 22, 2019 10:41

Fixed several layout issues with the material date picker. Mostly jus…

4cc086a

…t removed hard coded sizes to allow the grid view to scroll instead of overflowing.

Merge remote-tracking branch 'upstream/master' into date_picker_layout

1ce97be

Added tests for smaller layouts and localizations.

550fd10

darrenaustin requested a review from HansMuller April 23, 2019 19:58

googlebot added the cla: yes label Apr 23, 2019

xster added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Apr 23, 2019

Simplified the tests and removed some over-engineered configuration s…

d082a3d

…tuff.

HansMuller reviewed Apr 26, 2019

View reviewed changes

darrenaustin added 4 commits April 26, 2019 12:17

Merge remote-tracking branch 'upstream/master' into date_picker_layout

798b761

Merge remote-tracking branch 'upstream/master' into date_picker_layout

1709a85

Updates from PR review:

7867a56

- Renamed the sizes used for testing and documented them with comments. - Added more comments around the tests to better describe them and reference the bugs they test for.

Removed some final hard code sizes and opted instead to have the land…

38bddd8

…scape picker always try to take up 2/3 of the space and give the other 3rd to the header.

HansMuller approved these changes May 3, 2019

View reviewed changes

darrenaustin merged commit 2d2edbf into flutter:master May 6, 2019

darrenaustin deleted the date_picker_layout branch May 7, 2019 17:40

AiHMin mentioned this pull request May 20, 2019

Calendar widget for iOS overflown by 5 pixels #32991

Closed

HansMuller mentioned this pull request Sep 6, 2019

Planned Material Library Features Jul-Sept 2019 #35088

Closed

15 tasks

HansMuller mentioned this pull request Oct 18, 2019

Upcoming Material Library Features #43035

Closed

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021


		void _screenConfigTests() {

		const Size kPixel1Portrait = Size(1070, 1770);


		group('screen configurations', () {

		const Size kPixel1Portrait = Size(1070, 1770);

Conversation

darrenaustin commented Apr 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Example of layout fixes

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

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

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!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

darrenaustin commented Apr 23, 2019 •

edited

Loading