[Fix]: showDateRangePicker with "large" helpText cutoffs the save button by zeqinjie · Pull Request #146049 · flutter/flutter

zeqinjie · 2024-03-31T09:14:36Z

Fix #146039

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.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

…ton (flutter#146039)

packages/flutter/test/material/date_range_picker_test.dart

…lutter into fix_146039

nate-thegrate

Sorry for my poor explanation earlier—I think it'd be best to keep the existing test as-is, and add another test to cover this bugfix.

// existing test, unchanged
testWidgets('Save and help text is used', (WidgetTester tester) async {
  helpText = 'help';
  saveText = 'make it so';
  await preparePicker(tester, (Future<DateTimeRange?> range) async {
    expect(find.text(helpText!), findsOneWidget);
    expect(find.text(saveText!), findsOneWidget);
  });
});

// we should add a test like this
testWidgets("long helpText string doesn't cut off the 'save' button", (WidgetTester tester) async {
  helpText = 'long string' * 20;
  saveText = 'make it so';
  await preparePicker(tester, (Future<DateTimeRange?> range) async {
    expect(find.text(helpText!), findsOneWidget);
    expect(find.text(saveText!), findsOneWidget);
  });
});

P.S. I don't have write access in this repository, so you'll need an approval from @MitchellGoodwin and 1 other person :)

MitchellGoodwin

LGTM. Thank you for putting this together!

justinmc

LGTM with nits, and assuming that the Google tests pass. This could break image diff tests I think.

justinmc · 2024-04-23T18:07:24Z

packages/flutter/test/material/date_range_picker_test.dart

    });
  });

+  testWidgets('Long helpText cant not cutoffs the save button', (WidgetTester tester) async {


'Long helpText does not cutoff the save button'

@justinmc Thanks for your review, I have updated the test.

justinmc · 2024-04-23T18:12:16Z

packages/flutter/test/material/date_range_picker_test.dart

+    saveText = 'make it so';
+    await preparePicker(tester, (Future<DateTimeRange?> range) async {
+      expect(find.text(helpText!), findsOneWidget);
+      expect(find.text(saveText!), findsOneWidget);


It looks like this test will fail on master due to layout overflow, not due to these expectations. So I would add this line here: expect(tester.takeException(), null);

@justinmc Thanks for your review, I have updated the test.

justinmc

Sorry, forgot to check "approve".

zeqinjie · 2024-04-25T07:47:54Z

Sorry, forgot to check "approve".

@justinmc Thank you , can your review again

MitchellGoodwin · 2024-04-26T19:52:56Z

packages/flutter/test/material/date_range_picker_test.dart

    await preparePicker(tester, (Future<DateTimeRange?> range) async {
-      expect(find.text(helpText!), findsOneWidget);
-      expect(find.text(saveText!), findsOneWidget);
+      expect(tester.takeException(), null);


I think it would be better to have the check you had before and this one. So both this .takeException() and the check for helpText and saveText.

I believe the check for helpText and saveText is already there; the most recent commit reverted the change to that test and added a new one instead.

Here's what the branch looks like currently:

flutter/packages/flutter/test/material/date_range_picker_test.dart

Lines 261 to 277 in 3c26c7d

testWidgets('Save and help text is used', (WidgetTester tester) async {

helpText = 'help';

saveText = 'make it so';

await preparePicker(tester, (Future<DateTimeRange?> range) async {

expect(find.text(helpText!), findsOneWidget);

expect(find.text(saveText!), findsOneWidget);

});

});

testWidgets('Long helpText does not cutoff the save button', (WidgetTester tester) async {

helpText = 'long helpText' * 100;

await preparePicker(tester, (Future<DateTimeRange?> range) async {

expect(tester.takeException(), null);

});

});

I meant that the test with the long help text should also still check for the save button. That old test should not be touched as well.

I misunderstood 😅

Yeah, that makes sense!

Suggested change

expect(tester.takeException(), null);

expect(tester.takeException(), null);

expect(find.text(helpText!), findsOneWidget);

expect(find.text(saveText!), findsOneWidget);

@MitchellGoodwin Thank you for the review, sorry for my misunderstanding, I have changed the test.

…save button (flutter/flutter#146049)

flutter/flutter@db8c475...b597dd2 2024-04-30 [email protected] Roll Flutter Engine from 2c1517bbf555 to 0ce67714ce4c (1 revision) (flutter/flutter#147585) 2024-04-30 [email protected] Roll Packages from 87a7c51 to cc47b06 (5 revisions) (flutter/flutter#147580) 2024-04-30 [email protected] Roll Flutter Engine from 1a6ef9957b24 to 2c1517bbf555 (1 revision) (flutter/flutter#147579) 2024-04-30 [email protected] Roll Flutter Engine from 64877c08661d to 1a6ef9957b24 (1 revision) (flutter/flutter#147576) 2024-04-30 [email protected] Roll Flutter Engine from 1d62704cb3d2 to 64877c08661d (1 revision) (flutter/flutter#147567) 2024-04-30 [email protected] Roll Flutter Engine from aace42365fc7 to 1d62704cb3d2 (2 revisions) (flutter/flutter#147566) 2024-04-30 [email protected] Roll Flutter Engine from ea6343846c62 to aace42365fc7 (2 revisions) (flutter/flutter#147563) 2024-04-30 [email protected] Roll Flutter Engine from c519e9dd182b to ea6343846c62 (1 revision) (flutter/flutter#147560) 2024-04-30 [email protected] Roll Flutter Engine from 27bb23221305 to c519e9dd182b (1 revision) (flutter/flutter#147557) 2024-04-30 [email protected] Roll Flutter Engine from 93ccb9ab2cda to 27bb23221305 (2 revisions) (flutter/flutter#147556) 2024-04-30 [email protected] Roll Flutter Engine from c093562677fa to 93ccb9ab2cda (1 revision) (flutter/flutter#147553) 2024-04-29 [email protected] Bump dependencies in Flutter (flutter/flutter#147546) 2024-04-29 [email protected] Add configurable hitTestBehavior to Scrollable (flutter/flutter#146403) 2024-04-29 [email protected] Fix wide `DatePicker` input mode button padding for Material 3 (flutter/flutter#147236) 2024-04-29 [email protected] Roll Flutter Engine from 13c7ac9376a5 to c093562677fa (1 revision) (flutter/flutter#147547) 2024-04-29 [email protected] Roll Flutter Engine from 5165c711771c to 13c7ac9376a5 (1 revision) (flutter/flutter#147542) 2024-04-29 [email protected] Roll Packages from dd01140 to 87a7c51 (2 revisions) (flutter/flutter#147539) 2024-04-29 [email protected] Roll Flutter Engine from 399837d38807 to 5165c711771c (2 revisions) (flutter/flutter#147537) 2024-04-29 [email protected] [Fix]: showDateRangePicker with "large" helpText cutoffs the save button (flutter/flutter#146049) 2024-04-29 [email protected] Roll Flutter Engine from 752b146df767 to 399837d38807 (10 revisions) (flutter/flutter#147532) 2024-04-29 [email protected] Add ability to disable `FloatingActionButton` scale and rotation animations using `FloatingActionButtonAnimator.noAnimation` (flutter/flutter#146126) 2024-04-29 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 752b146df767 to f4c20e97e6aa (2 revisions) (#147495)" (flutter/flutter#147502) 2024-04-28 [email protected] Roll Flutter Engine from 752b146df767 to f4c20e97e6aa (2 revisions) (flutter/flutter#147495) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…save button (flutter/flutter#146049)

[Fix]: showDateRangePicker with "large" helpText cutoffs the save but…

ff04846

…ton (flutter#146039)

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Mar 31, 2024

Merge branch 'master' into fix_146039

e458fd5

nate-thegrate reviewed Apr 1, 2024

View reviewed changes

packages/flutter/test/material/date_range_picker_test.dart Outdated Show resolved Hide resolved

zhengzeqin007 and others added 3 commits April 3, 2024 10:37

Update test(flutter#146049)

e70d4d8

Update test(flutter#146049)

06f41e0

Merge branches 'fix_146039' and 'fix_146039' of github.com:zeqinjie/f…

73c7dd8

…lutter into fix_146039

HansMuller requested a review from MitchellGoodwin April 5, 2024 21:34

zeqinjie requested a review from nate-thegrate April 21, 2024 09:02

nate-thegrate reviewed Apr 21, 2024

View reviewed changes

add test

d48b27a

MitchellGoodwin approved these changes Apr 23, 2024

View reviewed changes

justinmc reviewed Apr 23, 2024

View reviewed changes

justinmc approved these changes Apr 23, 2024

View reviewed changes

update test

3c26c7d

MitchellGoodwin reviewed Apr 26, 2024

View reviewed changes

update test

f624713

MitchellGoodwin added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 29, 2024

auto-submit bot merged commit aabe53c into flutter:master Apr 29, 2024

zeqinjie deleted the fix_146039 branch April 30, 2024 06:00

engine-flutter-autoroll mentioned this pull request Apr 30, 2024

Roll Flutter from db8c475cac11 to 12ad41ca9e43 (20 revisions) flutter/packages#6628

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 30, 2024

aabe53c [Fix]: showDateRangePicker with "large" helpText cutoffs the …

056193c

…save button (flutter/flutter#146049)

engine-flutter-autoroll mentioned this pull request Apr 30, 2024

Roll Flutter from db8c475cac11 to 456f0101fd26 (22 revisions) flutter/packages#6630

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 30, 2024

aabe53c [Fix]: showDateRangePicker with "large" helpText cutoffs the …

6773fb6

…save button (flutter/flutter#146049)

engine-flutter-autoroll mentioned this pull request Apr 30, 2024

Roll Flutter from db8c475cac11 to b597dd24cb97 (23 revisions) flutter/packages#6631

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 30, 2024

aabe53c [Fix]: showDateRangePicker with "large" helpText cutoffs the …

d6bbf5b

…save button (flutter/flutter#146049)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

aabe53c [Fix]: showDateRangePicker with "large" helpText cutoffs the …

ec2bfaa

…save button (flutter/flutter#146049)


	testWidgets('Save and help text is used', (WidgetTester tester) async {
	helpText = 'help';
	saveText = 'make it so';
	await preparePicker(tester, (Future<DateTimeRange?> range) async {
	expect(find.text(helpText!), findsOneWidget);
	expect(find.text(saveText!), findsOneWidget);
	});
	});

	testWidgets('Long helpText does not cutoff the save button', (WidgetTester tester) async {
	helpText = 'long helpText' * 100;
	await preparePicker(tester, (Future<DateTimeRange?> range) async {
	expect(tester.takeException(), null);
	});
	});

Conversation

zeqinjie commented Mar 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nate-thegrate left a comment

Choose a reason for hiding this comment

Uh oh!

MitchellGoodwin left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

zeqinjie Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

zeqinjie Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

zeqinjie commented Apr 25, 2024

Uh oh!

MitchellGoodwin Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MitchellGoodwin Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

nate-thegrate Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeqinjie Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zeqinjie commented Mar 31, 2024 •

edited

Loading

nate-thegrate Apr 26, 2024 •

edited

Loading

nate-thegrate Apr 26, 2024 •

edited

Loading