settings: Make page scrollable #1911

MritunjayTiwari14 · 2025-10-10T13:29:21Z

Before	After (Screenshot)

Video Recordings

Before

before.mp4

After (Normal)

after.mp4

After (Increased Text Size to unreasonably high in android settings to test edge case)

after_increased_text_size_too_much_in_android_settings_edge_case.mp4

Fixes #1904

gnprice · 2025-10-11T00:28:40Z

Thanks for taking care of this!

Before we can review this change, it will need a test.

Those screenshots and recordings are very helpful, thanks. One remark: there's nothing unreasonable about the large text size — people vary in how sharp their vision is, and for some people that setting is useful by making it possible to read the text on the screen.

MritunjayTiwari14 · 2025-10-11T07:03:10Z

Thank you @gnprice for reminding about the test. I have added the test for scrolling in the latest commit.

I have limited the screen size to 200x200px to test the scrolling effect as lower down to 100x100px caused some rendering issue while running the test.

chrisbobbe

Thanks! Comments below.

For the commit message:

settings: Add scroll for settings page

This commit replaced Column with
ListView Widget to enable scrolling effect.

It needs a Fixes #<issue number>. line, like other commits that fix issues in the tracker.
The sentence in the body isn't needed; it just restates what's already obvious from reading the diff.
For the summary line, "add scroll" sounds a little odd to me; how about settings: Make page scrollable.

chrisbobbe · 2025-10-23T22:18:19Z

test/widgets/settings_test.dart

    }, variant: TargetPlatformVariant({TargetPlatform.android, TargetPlatform.iOS}));
  });

+  group('SettingPageScrollBehavior', () {


This string looks like an identifier, like the name of a class or something, but then I don't find anything when I look for its definition. To be less confusing, we should just say "SettingsPage scroll behavior" or similar. (SettingsPage is the name of a class, so it doesn't add confusion to use that name.)

Also nit: the test groups are now in this order:

'ThemeSetting'

'BrowserPreference'

(this group)

'VisitFirstUnreadSetting'

'MarkReadOnScrollSetting'

To be more organized, let's put the new group before the groups that are about individual settings, instead of in the middle of them.

Yes, Moving it to the top seems reasonable here.

chrisbobbe · 2025-10-23T22:21:55Z

test/widgets/settings_test.dart

+      final lastElementFinder = GlobalSettingsStore.experimentalFeatureFlags.isNotEmpty
+          ? find.text("Experimental features")
+          : find.text("Mark messages as read on scroll");
+      check(lastElementFinder.evaluate().isEmpty).equals(true);


nit: how about check(lastElementFinder).findsNothing();

(Good thought to test that the row isn't visible before scrolling. We want to be sure that the scrolling is actually what causes the row to become visible, rather than some other factor, which could arise in the future, like the settings being presented in a different order than when we wrote this test.)

chrisbobbe · 2025-10-23T22:22:07Z

test/widgets/settings_test.dart

+
+      await tester.scrollUntilVisible(lastElementFinder, 100,
+        scrollable: find.byType(Scrollable));
+      check(lastElementFinder.evaluate().isEmpty).equals(false);


nit: how about `check(lastElementFinder).findsOne();

Seems more high level.

chrisbobbe · 2025-10-23T22:22:38Z

test/widgets/settings_test.dart

+      final lastElementFinder = GlobalSettingsStore.experimentalFeatureFlags.isNotEmpty
+          ? find.text("Experimental features")
+          : find.text("Mark messages as read on scroll");


Suggested change

final lastElementFinder = GlobalSettingsStore.experimentalFeatureFlags.isNotEmpty

? find.text("Experimental features")

: find.text("Mark messages as read on scroll");

final lastElementFinder = GlobalSettingsStore.experimentalFeatureFlags.isNotEmpty

? find.text("Experimental features")

: find.text("Mark messages as read on scroll");

chrisbobbe · 2025-10-23T22:31:20Z

test/widgets/settings_test.dart

  });

+  group('SettingPageScrollBehavior', () {
+    testWidgets('scroll settings list when screen size is small', (tester) async {


nit:

Suggested change

testWidgets('scroll settings list when screen size is small', (tester) async {

testWidgets('content is scrollable when taller than a screenful', (tester) async {

(Helps focus the reader on what the test's purpose is, and acknowledges that the test can still be helpful when the screen is normal-sized, in a future where the list of settings is much longer.)

MritunjayTiwari14 · 2025-10-24T08:51:25Z

Thank you @chrisbobbe for taking time to review, all the requested changes and some other nits have been pushed.

chrisbobbe · 2025-10-24T21:16:40Z

Bump on #1911 (review) :

It needs a Fixes #<issue number>. line, like other commits that fix issues in the tracker.

Bump on #1911 (comment) :

[…] we should just say "SettingsPage scroll behavior" or similar. (SettingsPage is the name of a class, so it doesn't add confusion to use that name.)

MritunjayTiwari14 · 2025-10-25T06:09:08Z

Thanks @chrisbobbe for pointing out the issue. The following has been corrected

Corrected the Class name SettingsPage
Added Fixes #1904 in the commit description.

chrisbobbe

Thanks!

From the Git history (see Greg's "secret" to using git log -p, you can see we tend to use

"Fixes #1904." at the top, or
"Fixes: #1904" at the bottom

but not "Fixes #1904". (I think we're converging on option 1, which is what the Flutter project uses?)

gnprice · 2025-10-27T21:48:11Z

Yeah, I like option 1 of those. (That's what Flutter upstream uses, and I like how it sets context for what the rest of the commit message will be about.)

It's probably past time we should take the old style guide from zulip-mobile, copy the parts that apply here, and add that to this repo. This will be a good thing to write there.

MritunjayTiwari14 · 2025-10-28T05:30:08Z

Thank you @chrisbobbe and @gnprice for the clarification, the commit description has been updated with the option 1 as recommended:

settings: Make page scrollable
Fixes #1904

chrisbobbe · 2025-10-28T19:13:42Z

"Fixes #1904."

MritunjayTiwari14 · 2025-10-29T02:28:34Z

Thank you @chrisbobbe for noticing the typo. Commit description changed to:

settings: Make page scrollable
Fixes #1904.

chrisbobbe · 2025-10-30T22:11:18Z

Marking for Greg's review; thank you.

gnprice

Thanks @MritunjayTiwari14 for taking care of this, and thanks @chrisbobbe for the previous reviews! Just small comments below.

gnprice · 2025-10-31T01:41:25Z

lib/widgets/settings.dart

      appBar: ZulipAppBar(
        title: Text(zulipLocalizations.settingsPageTitle)),
-      body: Column(children: [
+      body: ListView(children: [


settings: Make page scrollable Fixes #1904.

The body should be separated from the summary line by a blank line, so that it makes its own paragraph.

gnprice · 2025-10-31T01:42:26Z

test/widgets/settings_test.dart

+      await tester.scrollUntilVisible(lastElementFinder, 100,
+        scrollable: find.byType(Scrollable));


This value for the scrollable option is the default behavior, right? So can leave it out.

gnprice · 2025-10-31T01:44:08Z

test/widgets/settings_test.dart

+  group('SettingsPage scroll behavior', () {
+    testWidgets('content is scrollable when taller than a screenful', (tester) async {


nit: can simplify by not wrapping this test in a group (since it was the only thing there anyway):

Suggested change

group('SettingsPage scroll behavior', () {

testWidgets('content is scrollable when taller than a screenful', (tester) async {

testWidgets('SettingsPage content is scrollable when taller than a screenful', (tester) async {

bump — I do think the "SettingsPage" part is helpful, for setting context when this test case goes by in flutter test output

MritunjayTiwari14 · 2025-10-31T05:18:08Z

Thank you @gnprice for the review. The following has been changed in the latest commit:

Unwrapped the test content is scrollable when taller than a screenful out of group SettingsPage scroll behavior and removed the group.
Added a new line between the summary and body so that Fixes #1904. appears at the top of body not along with summary.
Removed scrollable from tester.scrollUntilVisible since flutter automatically finds the Scrollable by default for scrollable.

MritunjayTiwari14 · 2025-11-01T06:00:59Z

Yes @gnprice, it would be right to provide context instead of just saying content, even though the test belong to SettingsPage . The context of content within SettingsPage can vary. So in the latest changes the following has been accommodated:

Changed content to SettingsPage to provide a better context of test.

Fixes zulip#1904.

gnprice · 2025-11-03T21:27:55Z

Thanks! Looks good; merging.

MritunjayTiwari14 force-pushed the issue-1904 branch from 499c590 to 4685d90 Compare October 11, 2025 07:00

gnprice added the maintainer review PR ready for review by Zulip maintainers label Oct 15, 2025

chrisbobbe self-assigned this Oct 23, 2025

chrisbobbe reviewed Oct 23, 2025

View reviewed changes

MritunjayTiwari14 force-pushed the issue-1904 branch from 4685d90 to 08e169b Compare October 24, 2025 08:49

MritunjayTiwari14 requested a review from chrisbobbe October 24, 2025 08:51

MritunjayTiwari14 force-pushed the issue-1904 branch from 08e169b to b5dea17 Compare October 25, 2025 06:07

chrisbobbe requested changes Oct 27, 2025

View reviewed changes

chrisbobbe mentioned this pull request Oct 27, 2025

Settings page should scroll #1904

Closed

MritunjayTiwari14 force-pushed the issue-1904 branch from b5dea17 to 832f918 Compare October 28, 2025 05:19

MritunjayTiwari14 requested a review from chrisbobbe October 28, 2025 05:25

MritunjayTiwari14 force-pushed the issue-1904 branch from 832f918 to a842403 Compare October 29, 2025 02:27

chrisbobbe requested a review from gnprice October 30, 2025 22:11

chrisbobbe assigned gnprice and unassigned chrisbobbe Oct 30, 2025

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Oct 30, 2025

gnprice reviewed Oct 31, 2025

View reviewed changes

MritunjayTiwari14 force-pushed the issue-1904 branch from a842403 to f2d5b48 Compare October 31, 2025 05:12

MritunjayTiwari14 requested a review from gnprice October 31, 2025 05:18

MritunjayTiwari14 force-pushed the issue-1904 branch 2 times, most recently from 187fce7 to ef31267 Compare November 1, 2025 05:59

settings: Make page scrollable

6e49e5c

Fixes zulip#1904.

gnprice force-pushed the issue-1904 branch from ef31267 to 6e49e5c Compare November 3, 2025 21:27

gnprice merged commit 6e49e5c into zulip:main Nov 3, 2025
1 check passed

MritunjayTiwari14 deleted the issue-1904 branch November 4, 2025 04:28

MritunjayTiwari14 changed the title ~~settings: Add scroll for settings page~~ settings: Make page scrollable Nov 4, 2025

	testWidgets('scroll settings list when screen size is small', (tester) async {
	testWidgets('content is scrollable when taller than a screenful', (tester) async {

		await tester.scrollUntilVisible(lastElementFinder, 100,
		scrollable: find.byType(Scrollable));

		group('SettingsPage scroll behavior', () {
		testWidgets('content is scrollable when taller than a screenful', (tester) async {

	group('SettingsPage scroll behavior', () {
	testWidgets('content is scrollable when taller than a screenful', (tester) async {
	testWidgets('SettingsPage content is scrollable when taller than a screenful', (tester) async {

settings: Make page scrollable #1911

settings: Make page scrollable #1911

Conversation

MritunjayTiwari14 commented Oct 10, 2025

Video Recordings

Before

After (Normal)

After (Increased Text Size to unreasonably high in android settings to test edge case)

Uh oh!

gnprice commented Oct 11, 2025

Uh oh!

MritunjayTiwari14 commented Oct 11, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Oct 23, 2025 • 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!

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!

MritunjayTiwari14 Oct 24, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MritunjayTiwari14 commented Oct 24, 2025

Uh oh!

chrisbobbe commented Oct 24, 2025

Uh oh!

MritunjayTiwari14 commented Oct 25, 2025

Uh oh!

chrisbobbe left a comment • 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!

gnprice commented Oct 27, 2025

Uh oh!

MritunjayTiwari14 commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisbobbe commented Oct 28, 2025

Uh oh!

MritunjayTiwari14 commented Oct 29, 2025

Uh oh!

chrisbobbe commented Oct 30, 2025

Uh oh!

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

MritunjayTiwari14 commented Oct 31, 2025

Uh oh!

MritunjayTiwari14 commented Nov 1, 2025

Uh oh!

gnprice commented Nov 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

chrisbobbe Oct 23, 2025 •

edited

Loading

MritunjayTiwari14 Oct 24, 2025 •

edited

Loading

chrisbobbe left a comment •

edited

Loading

MritunjayTiwari14 commented Oct 28, 2025 •

edited

Loading