diff --git a/TODO.md b/TODO.md index b0b27cd..56d6b20 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,7 @@ ## Frontend +- [ ] The notifications list should not refresh coldly every time. It should add/remove notifications with an animation as if a stack - [x] Make gruvbox the default theme - [x] Can we add front-end tests? - [x] Break down oott_api.dart in modules diff --git a/frontend/lib/devices/device_list.dart b/frontend/lib/devices/device_list.dart index cda0d01..ee53a1c 100644 --- a/frontend/lib/devices/device_list.dart +++ b/frontend/lib/devices/device_list.dart @@ -41,6 +41,7 @@ class _DeviceListState extends State with RouteAware { int _currentPage = 0; bool _hasNextPage = false; CancelToken? _fetchToken; + final ScrollController _scrollController = ScrollController(); @override void initState() { @@ -69,9 +70,23 @@ class _DeviceListState extends State with RouteAware { _ownerDebounce?.cancel(); _fetchToken?.cancel(); _ownerController.dispose(); + _scrollController.dispose(); super.dispose(); } + /// Fetches [page] and scrolls back to the top of the list, so changing pages + /// always starts the new page from its first row. + void _goToPage(int page) { + if (_scrollController.hasClients) { + _scrollController.animateTo( + 0, + duration: const Duration(milliseconds: 300), + curve: Curves.easeOut, + ); + } + _fetchPage(page); + } + void _onOwnerChanged() { _ownerDebounce?.cancel(); _ownerDebounce = Timer( @@ -286,6 +301,7 @@ class _DeviceListState extends State with RouteAware { return RefreshIndicator( onRefresh: () => _fetchPage(_currentPage), child: CustomScrollView( + controller: _scrollController, physics: const AlwaysScrollableScrollPhysics(), slivers: [ if (isWide) @@ -323,7 +339,7 @@ class _DeviceListState extends State with RouteAware { currentPage: _currentPage, hasNextPage: _hasNextPage, isLoading: _isLoading, - onPageChanged: _fetchPage, + onPageChanged: _goToPage, ), ), ], diff --git a/frontend/lib/home/notifications_list.dart b/frontend/lib/home/notifications_list.dart index 931f32b..298874d 100644 --- a/frontend/lib/home/notifications_list.dart +++ b/frontend/lib/home/notifications_list.dart @@ -52,6 +52,7 @@ class _NotificationsListState extends State bool _hasNextPage = false; String? _error; CancelToken? _fetchToken; + final ScrollController _scrollController = ScrollController(); @override void initState() { @@ -107,9 +108,23 @@ class _NotificationsListState extends State routeObserver.unsubscribe(this); _notificationTimer?.cancel(); _fetchToken?.cancel(); + _scrollController.dispose(); super.dispose(); } + /// Fetches [page] and scrolls back to the top of the list, so changing pages + /// always starts the new page from its first item. + void _goToPage(int page) { + if (_scrollController.hasClients) { + _scrollController.animateTo( + 0, + duration: const Duration(milliseconds: 300), + curve: Curves.easeOut, + ); + } + _fetchPage(page); + } + Future _fetchPage(int page) async { _fetchToken?.cancel(); final token = CancelToken(); @@ -207,6 +222,7 @@ class _NotificationsListState extends State _buildNotificationsHeader(context), Expanded( child: CustomScrollView( + controller: _scrollController, slivers: [ ..._buildNotificationSlivers(context), ...widget.trailingSlivers, @@ -296,7 +312,7 @@ class _NotificationsListState extends State currentPage: _currentPage, hasNextPage: _hasNextPage, isLoading: _isLoading, - onPageChanged: _fetchPage, + onPageChanged: _goToPage, ), ), ]; diff --git a/frontend/test/widget/device_list_test.dart b/frontend/test/widget/device_list_test.dart index 77c6b9d..b7139bd 100644 --- a/frontend/test/widget/device_list_test.dart +++ b/frontend/test/widget/device_list_test.dart @@ -1,3 +1,4 @@ +import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:frontend/devices/device_list.dart'; import 'package:frontend/devices/device_list_rows.dart'; @@ -31,8 +32,9 @@ void main() { await tearDownTree(tester); }); - testWidgets('shows the empty message when there are no devices', - (tester) async { + testWidgets('shows the empty message when there are no devices', ( + tester, + ) async { adapter.onGet('/devices', (server) => server.reply(200, [])); await pumpScreen(tester, const DeviceList()); @@ -57,4 +59,45 @@ void main() { await tearDownTree(tester); }); + + testWidgets('changing pages scrolls back to the top of the list', ( + tester, + ) async { + // 11 devices: a full page of 10 plus one, so the pagination bar appears. + adapter.onGet( + '/devices', + (server) => server.reply( + 200, + List.generate( + 11, + (i) => deviceJson( + macAddress: '00:00:00:00:00:${i.toString().padLeft(2, '0')}', + ), + ), + ), + ); + + // A short viewport so the rows overflow the screen and the list can scroll. + await pumpScreen(tester, const DeviceList(), size: const Size(900, 500)); + await pumpUntilFound(tester, find.byType(DeviceRowWide)); + + final scrollable = find.descendant( + of: find.byType(CustomScrollView), + matching: find.byType(Scrollable), + ); + ScrollPosition position() => + tester.state(scrollable).position; + + // Scroll down to reveal the pagination bar. + await tester.drag(scrollable, const Offset(0, -2000)); + await tester.pumpAndSettle(); + expect(position().pixels, greaterThan(0)); + + // Advance a page: the list should jump back to the top. + await tester.tap(find.byTooltip('Next page')); + await tester.pumpAndSettle(); + expect(position().pixels, 0); + + await tearDownTree(tester); + }); } diff --git a/frontend/test/widget/notifications_list_test.dart b/frontend/test/widget/notifications_list_test.dart index 09e9709..5575734 100644 --- a/frontend/test/widget/notifications_list_test.dart +++ b/frontend/test/widget/notifications_list_test.dart @@ -54,4 +54,55 @@ void main() { await tester.pumpWidget(const SizedBox()); }, ); + + testWidgets('changing pages scrolls back to the top of the list', ( + tester, + ) async { + tester.view.physicalSize = const Size(400, 300); + tester.view.devicePixelRatio = 1.0; + addTearDown(tester.view.resetPhysicalSize); + + List> page(int firstId) => List.generate( + 6, + (i) => notificationJson(id: firstId + i, title: 'Item ${firstId + i}'), + ); + adapter.onGet( + '/notifications', + (server) => server.reply(200, page(1)), + queryParameters: {'is_new': true, 'page_offset': 0, 'page_limit': 6}, + ); + adapter.onGet( + '/notifications', + (server) => server.reply(200, page(7)), + queryParameters: {'is_new': true, 'page_offset': 5, 'page_limit': 6}, + ); + + await tester.pumpWidget( + MaterialApp( + theme: gruvboxDarkTheme, + home: const Scaffold(body: NotificationsList()), + ), + ); + await tester.pumpAndSettle(); + + final scrollable = find.descendant( + of: find.byType(CustomScrollView), + matching: find.byType(Scrollable), + ); + ScrollPosition position() => + tester.state(scrollable).position; + + // Scroll down to reveal the pagination bar. + await tester.drag(scrollable, const Offset(0, -2000)); + await tester.pumpAndSettle(); + expect(position().pixels, greaterThan(0)); + + // Advance a page: the list should jump back to the top. + await tester.tap(find.byTooltip('Next page')); + await tester.pumpAndSettle(); + expect(find.textContaining('Item 7'), findsWidgets); + expect(position().pixels, 0); + + await tester.pumpWidget(const SizedBox()); + }); }