app-responsiveness: fix renderflex overfow in widgets. #988
Conversation
widgets. Fix renderflex overflow in the Home, Add Account and AlertDialog widgets, by making them scrollable, when all contents in a screen do not fit in the screen.
|
The before and after images of the Homepage which has a renderflex overflow and when it was fixed.
The before and after images showing Add Account page with renderflex overflow and when it is fixed. |
|
Is there an issue or a previous conversation that prompted this PR? If so, please put a link in the PR description so we can review with all the relevant context; thanks. |
Yes, there is, this chat was what prompted the PR. |
test/widgets/app_test.dart
Outdated
| testWidgets('Home scrollable test, when contents do not fit to screen', (tester)async{ | ||
| await tester.pumpWidget(MaterialApp( | ||
| home: Scaffold(body: Center(child: SingleChildScrollView(child: Column( | ||
| children: List.generate(15, (count)=>Text('Pick $count')), | ||
| )))))); | ||
| //make sure the added scrollable widget is functional | ||
| expect(find.byType(SingleChildScrollView), findsOneWidget); | ||
| expect(find.text('Pick 0'), findsOneWidget); | ||
|
|
||
| await tester.drag(find.byType(SingleChildScrollView), const Offset(0, -300)); | ||
| //Test for items down the column | ||
| await tester.pump(); | ||
| expect(find.text('Pick 14'), findsOneWidget); | ||
| }); |
There was a problem hiding this comment.
This is meant to test the code you added in HomePage, right? So it should exercise that code itself, instead of writing code for a new page that doesn't appear in the app. :)
To make it so the content runs longer than a screenful, try making the test simulate a shorter device height, and/or simulate an increased device text-size setting. We have examples of both in other widget tests.
(Similarly for AddAccountPage.)
There was a problem hiding this comment.
Thank you for pointing this out, I will make the changes and send a PR
Co-authored-by: Chris Bobbe <[email protected]>
Co-authored-by: Chris Bobbe <[email protected]>
Co-authored-by: Chris Bobbe <[email protected]>
widgets. Fix renderflex overflow in the Home, Add Account and AlertDialog widgets, by making them scrollable, when all contents in a screen do not fit in the screen.
Remove the SingleChildScrollView in the content field: Pass scrollable: true to the AlertDialog constructor
…rflex Overflow on smaller screens, and write corresponding test for the change.
|
Hello, I have applied the changes, and waiting for review. |
|
Hi @oriohac, please note our convention of "clear and coherent commits"; you'll need to tidy up your commit history before we can review your PR in more detail. Some things I notice happening:
Also, bump on #988 (comment), which is still unresolved. It looks like you've copy-pasted from the current implementation of |
As a result, showErrorDialog no longer returns a Future that can be mistakenly awaited. The wrapped return value offers clarity on how the Future is supposed to be used. There is no user visible change because existing callers (other than lightbox's) that wait for its return value are all asynchronous handlers for UI elements like buttons, and waits unnecessarily. See also discussion zulip#937 (comment). Signed-off-by: Zixuan James Li <[email protected]>
Unlike ScaffoldFeatureController which inspired it, this class doesn't offer a way to control the dialog, so it's not really a "controller"; just call it DialogStatus. A private final field with a public getter (of the same type) is equivalent to just a public final field: nothing can set it, and any library can get it. Also generalize the description a bit: we currently use this only for AlertDialog, but it'd apply equally well for any dialog box.
widgets. Fix renderflex overflow in the Home, Add Account and AlertDialog widgets, by making them scrollable, when all contents in a screen do not fit in the screen.
|
This is not ready for another review. Please see above: #988 (comment) and ask in |
Fix renderflex overflow in the Home, Add Account
and AlertDialog widgets, by making them scrollable, when all contents in a screen do not fit in the
screen. Which are fixes to these observed issues.