Skip to content
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

accounts-ui: Make accounts list scrollable #593

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

sm-sayedi
Copy link
Collaborator

Before this fix, the list of accounts did not scroll when they were more than a screenful and would show an overflow error on the screen.

After this fix, the list of accounts scrolls properly with no overflow error and without shifting other content offscreen.

Before After

Fixes: #100

@sm-sayedi sm-sayedi force-pushed the issue-100-scrollable-accounts-list branch 3 times, most recently from 8bf59c5 to 5c62d37 Compare March 27, 2024 09:47
@sm-sayedi sm-sayedi marked this pull request as ready for review March 27, 2024 09:50
@sm-sayedi sm-sayedi force-pushed the issue-100-scrollable-accounts-list branch 3 times, most recently from 0614b90 to f752479 Compare March 27, 2024 16:43
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sm-sayedi! Generally this looks great; several small comments on the tests.

test/widgets/app_test.dart Show resolved Hide resolved
test/widgets/app_test.dart Outdated Show resolved Hide resolved
test/widgets/app_test.dart Outdated Show resolved Hide resolved
test/widgets/app_test.dart Outdated Show resolved Hide resolved
test/widgets/app_test.dart Show resolved Hide resolved
@sm-sayedi sm-sayedi force-pushed the issue-100-scrollable-accounts-list branch from f752479 to a246461 Compare March 28, 2024 07:10
@sm-sayedi
Copy link
Collaborator Author

Thanks for the review! Revision pushed. PTAL!

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the revision! Generally this looks great; a few small comments below.


group('ChooseAccountPage', () {
Future<void> setupChooseAccountPage(WidgetTester tester, {
required List<Account>? accounts,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: requiring the caller to explicitly pass this makes sense, but then it may as well just take a list:

Suggested change
required List<Account>? accounts,
required List<Account> accounts,

so the caller explicitly says [], instead of saying null and having that mean [].

(In fact, now that I think of that, even in the version where this parameter was optional it probably should have been List<Account> accounts = [], rather than being nullable.)

Comment on lines 108 to 109
testWidgets('accounts list is scrollable when more than a screenful',
(tester) async {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we just leave the (tester) async { part on the same line even if that makes the line long:

Suggested change
testWidgets('accounts list is scrollable when more than a screenful',
(tester) async {
testWidgets('accounts list is scrollable when more than a screenful', (tester) async {

Because that part is totally boring and predictable, it's OK if it gets scrolled off screen for the reader.

Comment on lines 108 to 110
testWidgets('accounts list is scrollable when more than a screenful',
(tester) async {
final accounts = generateAccounts(15);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If OTOH we had a situation where it wasn't so boring and predictable and did need to go on the next line, we'd want to avoid having the (tester) async { aligned with the body, because it makes it look like part of the body. For a function, the standard way to do that is to indent the body:

    testWidgets('accounts list is scrollable when more than a screenful',
      (tester) async {
        final accounts = generateAccounts(15);

)

Comment on lines 141 to 143
final accountRect = tester.getRect(findAccount(account));
check(accountRect.top).isGreaterThan(thirdOfScreenHeight);
check(accountRect.bottom).isLessThan(screenHeight - thirdOfScreenHeight);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's write this in more thoroughly package:checks style:

Suggested change
final accountRect = tester.getRect(findAccount(account));
check(accountRect.top).isGreaterThan(thirdOfScreenHeight);
check(accountRect.bottom).isLessThan(screenHeight - thirdOfScreenHeight);
check(tester.getRect(findAccount(account)))
..top.isGreaterThan(thirdOfScreenHeight)
..bottom.isLessThan(screenHeight - thirdOfScreenHeight);

You'll need to write those accessors top and bottom , since we haven't yet. They'll go in test/flutter_checks.dart; see existing examples there.

Comment on lines 142 to 143
check(accountRect.top).isGreaterThan(thirdOfScreenHeight);
check(accountRect.bottom).isLessThan(screenHeight - thirdOfScreenHeight);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately, for the numbers, let's just write:

Suggested change
check(accountRect.top).isGreaterThan(thirdOfScreenHeight);
check(accountRect.bottom).isLessThan(screenHeight - thirdOfScreenHeight);
check(accountRect.top).isGreaterThan(1/3 * screenHeight);
check(accountRect.bottom).isLessThan(2/3 * screenHeight);

For me at least, that's clearer to read than the more word-based version.

@sm-sayedi sm-sayedi force-pushed the issue-100-scrollable-accounts-list branch from a246461 to 680b9ff Compare March 29, 2024 05:45
@sm-sayedi
Copy link
Collaborator Author

Thanks for the review! Pushed the revision. Please have a look!

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit below; otherwise all looks good. I'll fix that nit and merge. Thanks @sm-sayedi for adding this!

Comment on lines 13 to 15
}
extension AnimationChecks<T> on Subject<Animation<T>> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
}
extension AnimationChecks<T> on Subject<Animation<T>> {
}
extension AnimationChecks<T> on Subject<Animation<T>> {

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Before this fix, the list of accounts did not scroll
when they were more than a screenful and would
show an overflow error on the screen.

After this fix, the list of accounts scrolls properly
with no overflow error and without shifting other
content offscreen.

Fixes: zulip#100
@gnprice gnprice force-pushed the issue-100-scrollable-accounts-list branch from 680b9ff to e6c531a Compare March 29, 2024 20:51
@gnprice gnprice merged commit e6c531a into zulip:main Mar 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

accounts ui: List of accounts doesn't scroll when more than a screenful
2 participants