msglist: add scroll-to-bottom button to MessageList #223

sirpengi · 2023-07-18T23:16:16Z

Introducing a scroll-to-bottom button in the MessageList component that shows up only if the user is not at the bottom of the list. Clicking on that button triggers a scroll to the bottom.

Notes

My initial version wrapped the MessageList with another component that controlled scrolling, but I was able to fix the re-rendering issue while putting integrating the functionality right in the component. A benefit of having wrapper component may be to reuse the scroll-to-bottom feature but I don't believe that is used in any other context (the list of streams or users do not have this).
I did not add a test case yet, I would prefer to wait for a greenlight on the implemented component as the test case would need some knowledge of implementation to trigger and check results

sirpengi · 2023-07-18T23:20:16Z

@gnprice @chrisbobbe Looking for some draft-stage comments on this!

gnprice

Thanks @sirpengi! Generally this direction looks good — I think the only comment on the high-level direction is about how to listen for scroll events. Please see that and a few other comments below, and a couple of tests would also be very helpful.

It looks like we don't currently have any tests for this MessageList widget. I think at this point we have all the test infrastructure to be able to write tests for it, but there might still be something we're missing — please ask in #mobile-team if you run into any trouble.

lib/widgets/message_list.dart

gnprice

Thanks @sirpengi! I've now gone through the code for a full review; detailed comments below.

I think this structure is working well, and I'm happy to see that the tests turned out pretty clean too. Most of the comments I have are stylistic, and many of those are minor nits; there's a few that are more substantive, but I don't think any of them will be complex to deal with.

gnprice · 2023-07-21T00:00:54Z

test/widgets/message_list_test.dart

+		testWidgets('scrolling changes visibility', (WidgetTester tester) async {
+			final stream = eg.stream();
+			await setupMessageListPage(tester, narrow: StreamNarrow(stream.streamId));
+
+      final scrollController = findMessageListScrollController(tester)!;


It looks like there are some tab characters in here. I don't actually notice when reading in Android Studio — I guess I have tab stops of 2 columns and that must be the same as you were writing with — but it makes things harder to read on GitHub and in git log.

gnprice · 2023-07-21T00:01:07Z

test/widgets/message_list_test.dart

+    }
+		ScrollToBottomButton? findScrollToBottomButton(WidgetTester tester) {


nit: separate these with a blank line

gnprice · 2023-07-21T00:01:26Z

test/widgets/message_list_test.dart

+
+  group('ScrollToBottomButton interactions', () {
+    ScrollController? findMessageListScrollController(WidgetTester tester) {
+      final stickyHeaderListView = tester.widget<StickyHeaderListView>(find.byType(StickyHeaderListView))!;


analyzer complains this ! is redundant

gnprice · 2023-07-21T00:03:04Z

test/widgets/message_list_test.dart

+/// Prepare a [MessageListPage]
+Future<void> setupMessageListPage(WidgetTester tester, {


nit: This comment doesn't really add information that isn't in the name, so better to leave out.

gnprice · 2023-07-21T00:11:25Z

test/widgets/message_list_test.dart

+      final scrollToBottomButton = findScrollToBottomButton(tester)!;
+
+      // Initial state should be not visible, as the message list renders with latest message in view
+      check(scrollToBottomButton.visibleValue.value).equals(false);


Rather than look at visibleValue, I'd prefer to have the test check more directly that the button is visible, or isn't. That seems like it's the real contract we're asking this part of the UI logic to fulfill; so testing at that layer lets visibleValue (and even the ScrollToBottomButton class, I think) be an implementation detail that can be freely refactored. It also enables the test to catch bugs in a wider swath of the code, in case some future change were to break something in the connection from visibleValue to the actual showing of the button.

I think find.byTooltip gives a good way to do that.

gnprice · 2023-07-21T00:38:58Z

lib/widgets/message_list.dart

+                    Positioned(
+                      bottom: 0,
+                      right: 0,
+                      child: ScrollToBottomButton(scrollController: scrollController, visibleValue: _scrollToBottomVisibleValue),


nit: this line is awfully long — let's break the arguments onto their own lines

gnprice · 2023-07-21T01:14:58Z

lib/widgets/message_list.dart

+  final ScrollController scrollController = ScrollController();
+
+  final ValueNotifier<bool> _scrollToBottomVisibleValue = ValueNotifier<bool>(false);


Since we're having this state create these resources, let's have it dispose them in its own dispose method.

I'm not sure that ultimately affects anything in this case — maybe nothing else is left that has a reference to them by the time this state gets disposed — but I don't know a reason to be sure it doesn't matter here, and the pattern "always dispose all the state's resources in dispose" seems like a good one to consistently follow because sometimes it does matter.

gnprice · 2023-07-21T02:58:25Z

lib/widgets/message_list.dart

+    // Calculate time necessary if we're capping speed at 8pixels/ms (== 8000pixels/s)
+    final speedLimit = (distance / 8).ceil();
+    final int durationMs = max(300, speedLimit);
+    scrollController.animateTo(0, duration: Duration(milliseconds: durationMs), curve: Curves.easeIn);


nit: break these arguments onto separate lines — in particular the value for curve is a meaningful UI choice, and it's easy for it to get lost off on the right past 80 columns

gnprice · 2023-07-21T03:01:06Z

lib/widgets/message_list.dart

+    // Calculate time necessary if we're capping speed at 8pixels/ms (== 8000pixels/s)
+    final speedLimit = (distance / 8).ceil();
+    final int durationMs = max(300, speedLimit);
+    scrollController.animateTo(0, duration: Duration(milliseconds: durationMs), curve: Curves.easeIn);


Can you tell me about your reasoning for picking Curves.easeIn?

I hate to admit I didn't review all the curve behaviors fully. I did notice that the web behavior is to jump to the bottom and the mobile behavior seems linear. In my previous work with UI I've always found ease-type functions to feel more natural. I must admit my 300ms timer was also a sort of "default practice" for UI based on previous work with animation timings. To evaluate this topic more, I've looked over the material design guidance for motion, see https://m2.material.io/design/motion/speed.html#controlling-speed and https://m3.material.io/styles/motion/easing-and-duration/applying-easing-and-duration and while I got the 300ms perfectly right for their recommended default animation they both recommend a quick-start and slow down for easing, which I believe maps more to the Curves.ease rather than Curves.easeIn. I'd conclude I should switch this to Curves.ease instead to match? Unless there is another style guide to consider in this design?

Cool. Yeah, that fast-start, slow-finish curve sounds good.

gnprice · 2023-07-21T03:29:44Z

test/widgets/message_list_test.dart

+void main() {
+  TestZulipBinding.ensureInitialized();
+
+  group('ScrollToBottomButton interactions', () {


Let's have one more test case — testing that the button actually works :-)

gnprice

Thanks @sirpengi for the revision! This is very close to merge now — one substantive comment on the new test case, and a few nits.

See below, and also two small things above that remain open:
#223 (comment)
#223 (comment)

When I merge this, I'll also squash all the commits into one, because ultimately they're all revisions to a single coherent change.

Many PRs end up as several commits because they make preparatory refactors, or add a feature in one place and then another feature that builds on that; in order to support those, our usual workflow is that each revision is a new draft of the whole final sequence of commits to merge, which means the author does this sort of squashing. (That does cause GitHub to be less helpful about comparing different revisions; the reviewer instead relies on having fetched a local copy, and we have a couple of scripts to help make that easy.)

But for this PR, go ahead and carry on adding commits on top. Squashing at merge time is easy when it's all going to be a single commit (as this is), and having gone most of the way in this style it's probably simplest all around to finish it that way.

gnprice · 2023-07-24T21:14:10Z

test/widgets/message_list_test.dart

+
+      await tester.tap(find.byType(ScrollToBottomButton));
+      await tester.pumpAndSettle();
+      check(isButtonVisible(tester)).equals(false);


Let's check instead that the scroll controller's actual position is at the bottom — that way this test wouldn't accidentally pass if we introduced some bug where the button didn't actually work, but did cause the button to go away 🙂

gnprice · 2023-07-24T21:16:16Z

test/widgets/message_list_test.dart

+      check(isButtonVisible(tester)).equals(false);
+    });
+  });
+}


nit: missing final newline

gnprice · 2023-07-24T21:30:50Z

lib/widgets/message_list.dart

+                      right: 0,
+                      child: ScrollToBottomButton(
+                        scrollController: scrollController,
+                        visibleValue: _scrollToBottomVisibleValue))])))))));


nit:

Suggested change

visibleValue: _scrollToBottomVisibleValue))])))))));

visibleValue: _scrollToBottomVisibleValue)),

])))))));

(For lists […] we end the last item with comma-newline just like the other items, while for child arguments and many other function arguments, as you've seen, we stack up parens on the final line.

This reflects the fact that the different elements of a list one often wants to think of as relatively equal/symmetric siblings, and one may well remove the last element or insert more after it; whereas a child argument one thinks of differently from the more metadata-like other arguments, and we'll never insert another argument after a widget's child.)

sirpengi · 2023-07-28T18:07:38Z

@gnprice okay I believe this is ready again, with all comments addressed. I've rebased onto an updated main as well

gnprice

Great — this all looks good, with just one nit below which I notice on re-read. I'll fix that, squash as discussed above, and merge. Thanks again @sirpengi for all your work on this!

gnprice · 2023-07-31T23:11:33Z

test/widgets/message_list_test.dart

+import '../model/binding.dart';
+
+
+Future<void> setupMessageListPage(WidgetTester tester, {


nit:

Suggested change

import '../model/binding.dart';

Future<void> setupMessageListPage(WidgetTester tester, {

import '../model/binding.dart';

Future<void> setupMessageListPage(WidgetTester tester, {

sirpengi force-pushed the pr-scroll-to-bottom-button branch from 013c739 to fdc9d0c Compare July 18, 2023 23:17

gnprice reviewed Jul 19, 2023

View reviewed changes

sirpengi marked this pull request as ready for review July 19, 2023 22:48

gnprice mentioned this pull request Jul 19, 2023

Pin a precise version of Flutter #15

Open

gnprice reviewed Jul 21, 2023

View reviewed changes

sirpengi force-pushed the pr-scroll-to-bottom-button branch from e8b9cca to fccf8c6 Compare July 21, 2023 23:04

gnprice reviewed Jul 24, 2023

View reviewed changes

sirpengi force-pushed the pr-scroll-to-bottom-button branch from f556b8d to e4cf32b Compare July 28, 2023 18:00

msglist: Add a scroll-to-bottom button

5abeb88

gnprice reviewed Jul 31, 2023

View reviewed changes

gnprice force-pushed the pr-scroll-to-bottom-button branch from e4cf32b to 5abeb88 Compare July 31, 2023 23:19

gnprice merged commit 5abeb88 into zulip:main Jul 31, 2023

gnprice mentioned this pull request Aug 1, 2023

icons: Add a custom icon font #219

Merged

sirpengi mentioned this pull request Aug 11, 2023

msglist: Scroll-to-bottom button on message list #218

Closed

sirpengi deleted the pr-scroll-to-bottom-button branch January 23, 2024 14:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

msglist: add scroll-to-bottom button to MessageList #223

msglist: add scroll-to-bottom button to MessageList #223

sirpengi commented Jul 18, 2023

sirpengi commented Jul 18, 2023

gnprice left a comment

gnprice left a comment

gnprice Jul 21, 2023

gnprice Jul 21, 2023

gnprice Jul 21, 2023

gnprice Jul 21, 2023

gnprice Jul 21, 2023

gnprice Jul 21, 2023

gnprice Jul 21, 2023

gnprice Jul 21, 2023

gnprice Jul 21, 2023

sirpengi Jul 21, 2023 •

edited

Loading

gnprice Jul 21, 2023

gnprice Jul 21, 2023

gnprice left a comment

gnprice Jul 24, 2023

gnprice Jul 24, 2023

gnprice Jul 24, 2023

sirpengi commented Jul 28, 2023

gnprice left a comment

gnprice Jul 31, 2023

		}
		ScrollToBottomButton? findScrollToBottomButton(WidgetTester tester) {

		/// Prepare a [MessageListPage]
		Future<void> setupMessageListPage(WidgetTester tester, {

		final ScrollController scrollController = ScrollController();

		final ValueNotifier<bool> _scrollToBottomVisibleValue = ValueNotifier<bool>(false);

	visibleValue: _scrollToBottomVisibleValue))])))))));
	visibleValue: _scrollToBottomVisibleValue)),
	])))))));

		import '../model/binding.dart';


		Future<void> setupMessageListPage(WidgetTester tester, {

msglist: add scroll-to-bottom button to MessageList #223

msglist: add scroll-to-bottom button to MessageList #223

Conversation

sirpengi commented Jul 18, 2023

Notes

sirpengi commented Jul 18, 2023

gnprice left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Jul 28, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi Jul 21, 2023 •

edited

Loading