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.
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
Date picker layout exceptions #31514
Conversation
…t removed hard coded sizes to allow the grid view to scroll instead of overflowing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -493,6 +486,7 @@ class DayPicker extends StatelessWidget { | |||
child: GridView.custom( | |||
gridDelegate: _kDayPickerGridDelegate, | |||
childrenDelegate: SliverChildListDelegate(labels, addRepaintBoundaries: false), | |||
padding: EdgeInsets.zero, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we set this to zero don't we risk having the bottom of the grid end up behind the display's notch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -682,7 +676,6 @@ class _MonthPickerState extends State<MonthPicker> with SingleTickerProviderStat | |||
@override | |||
Widget build(BuildContext context) { | |||
return SizedBox( | |||
width: _kMonthPickerPortraitWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
void _screenConfigTests() { | ||
|
||
const Size kPixel1Portrait = Size(1070, 1770); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment about these sizes, like what they're based on, why we chose them, would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pushed a fix that renamed them and commented them with hopefully better descriptions.
@@ -774,3 +777,70 @@ void _tests() { | |||
expect(tester.getBottomLeft(find.text('OK')).dx, 800 - ltrOkRight); | |||
}); | |||
} | |||
|
|||
void _screenConfigTests() { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
} | ||
|
||
testWidgets('display on Pixel1 - portrait', (WidgetTester tester) async { | ||
await _showPicker(tester, kPixel1Portrait); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are verifying that we don't assert. Just to make that clear it would be good to append:
expect(tester.takeException(), isNull);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice suggestion. I have added this to all the tests.
@@ -223,6 +223,59 @@ void main() { | |||
|
|||
await tester.tap(find.text('ANNULER')); | |||
}); | |||
|
|||
group('screen configurations', () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What these tests have in common seems to be that we're varying locale?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :).
|
||
const Size kPixel1Portrait = Size(1070, 1770); | ||
const Size kPixel1Landscape = Size(1770, 1070); | ||
Future<void> _showPicker(WidgetTester tester, Locale locale, Size size) async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a blank line here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
group('screen configurations', () { | ||
|
||
const Size kPixel1Portrait = Size(1070, 1770); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As before, maybe there's something to say about the sizes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?