From dd80677e5a37ee11e76689bf42e6bcc8c751701f Mon Sep 17 00:00:00 2001 From: Leo Farias Date: Thu, 6 Aug 2026 10:21:56 -0400 Subject: [PATCH] fix(remix): data table review follow-ups --- docs/components/data_table.mdx | 3 +- packages/remix/README.md | 2 +- .../data_table/data_table_spec.dart | 5 + .../data_table/data_table_widget.dart | 66 +++++++++-- .../data_table/data_table_style_test.dart | 105 +++++++++++++++++- .../data_table/data_table_widget_test.dart | 25 +++++ 6 files changed, 192 insertions(+), 14 deletions(-) diff --git a/docs/components/data_table.mdx b/docs/components/data_table.mdx index c973ebc5..aa0f9333 100644 --- a/docs/components/data_table.mdx +++ b/docs/components/data_table.mdx @@ -301,7 +301,8 @@ Horizontal scrolling belongs to the table. Under a bounded width it lays out at `max(minimumWidth, availableWidth)` inside a horizontal viewport, so flex columns never resolve against unbounded constraints and a narrow viewport scrolls instead of overflowing. Vertical scrolling, sticky headers, and -viewport height stay with the parent. +viewport height stay with the parent. The pagination footer stays pinned to +the visible width while the header and body scroll. ## Accessibility diff --git a/packages/remix/README.md b/packages/remix/README.md index 40305f2a..32fa5ace 100644 --- a/packages/remix/README.md +++ b/packages/remix/README.md @@ -288,9 +288,9 @@ Remix provides a comprehensive set of production-ready components: - **Card** - Content containers - **DataList** - Label/value metadata lists with a shared label column - **DataTable** - Controlled tables with shared column headers, sorting, selection, and pagination -- **Skeleton** - Loading placeholders that mirror their content - **Divider** - Visual separators - **Progress** - Progress indicators +- **Skeleton** - Loading placeholders that mirror their content - **Spinner** - Loading states ### Layout & Navigation diff --git a/packages/remix/lib/src/components/data_table/data_table_spec.dart b/packages/remix/lib/src/components/data_table/data_table_spec.dart index d4d69e36..cb16840e 100644 --- a/packages/remix/lib/src/components/data_table/data_table_spec.dart +++ b/packages/remix/lib/src/components/data_table/data_table_spec.dart @@ -62,6 +62,11 @@ class DataTableSpec with _$DataTableSpec { /// Null keeps the last row identical to the others. Radix's surface variant /// is the one real consumer: it drops the trailing divider so the last row /// does not double up with the panel border. + /// + /// On the styler path the override merges over [bodyRow]; a raw + /// [RemixDataTable.styleSpec] carries resolved specs, which cannot merge, + /// so there a non-null value replaces [bodyRow] wholesale and must be + /// pre-composed by the caller. @override final StyleSpec? lastBodyRow; diff --git a/packages/remix/lib/src/components/data_table/data_table_widget.dart b/packages/remix/lib/src/components/data_table/data_table_widget.dart index 4a55f812..ecb78620 100644 --- a/packages/remix/lib/src/components/data_table/data_table_widget.dart +++ b/packages/remix/lib/src/components/data_table/data_table_widget.dart @@ -184,7 +184,8 @@ final class RemixDataTableLabels { /// table is laid out at `max(minimumWidth, availableWidth)` inside a /// horizontal viewport, so flex columns never resolve against unbounded /// constraints. Vertical scrolling, sticky headers, and viewport height stay -/// with the parent. +/// with the parent. The pagination footer stays pinned to the visible width +/// while the header and body scroll. /// /// ## Example /// @@ -355,6 +356,15 @@ class RemixDataTable extends StatelessWidget { 'RemixDataTable.minimumWidth must be a finite non-negative value.', ); + // StyleBuilder merges StyleProvider-inherited styles into the resolved + // spec; merge them here too so per-row re-resolution sees the same + // widget-state variants. Other Style subtypes (IdentityStyle) carry no + // props to re-resolve and are already represented in the spec fallback. + final inherited = Style.maybeOf(context); + final effectiveStyler = inherited is DataTableStyler + ? inherited.merge(style) + : style; + return RemixStyleSpecBuilder( style: style, styleSpec: styleSpec, @@ -368,7 +378,7 @@ class RemixDataTable extends StatelessWidget { // A supplied raw spec has no styler to re-resolve per row, so the // table-level values are the final ones in that path. styles: _DataTableStyles( - styler: styleSpec == null ? style : null, + styler: styleSpec == null ? effectiveStyler : null, spec: spec, ), ), @@ -464,6 +474,11 @@ class RemixDataTable extends StatelessWidget { if (total == null) return true; assert(total >= 0, 'RemixDataTable.totalRows must not be negative.'); assert(pageSize > 0, 'RemixDataTable.pageSize must be positive.'); + assert( + rows.length <= pageSize, + 'RemixDataTable.rows has ${rows.length} rows, which exceeds pageSize ' + '($pageSize): rows must be exactly the visible page.', + ); assert( sizeOptions.isNotEmpty && sizeOptions.every((option) => option > 0), 'RemixDataTable.pageSizeOptions must be nonempty and positive.', @@ -582,6 +597,18 @@ class _RemixDataTableView extends StatefulWidget { class _RemixDataTableViewState extends State<_RemixDataTableView> { int? _hoveredRow; + @override + void didUpdateWidget(covariant _RemixDataTableView oldWidget) { + super.didUpdateWidget(oldWidget); + // Hover is tracked by row index and MouseRegion.onExit does not fire for + // a region unmounted while hovered, so new row content invalidates the + // index. The mouse tracker re-enters the correct row on the next frame + // when the pointer is still over one. + if (_hoveredRow != null && !listEquals(widget.rows, oldWidget.rows)) { + _hoveredRow = null; + } + } + RemixDataTable get _table => widget.table; bool get _selectable => _table._selectable; @@ -670,7 +697,7 @@ class _RemixDataTableViewState extends State<_RemixDataTableView> { 'DataTableSpec dimensions must resolve to non-negative values.', ); - final content = Column( + final tableContent = Column( mainAxisSize: MainAxisSize.min, crossAxisAlignment: CrossAxisAlignment.stretch, children: [ @@ -681,7 +708,6 @@ class _RemixDataTableViewState extends State<_RemixDataTableView> { // semantics stay caller-owned. if (widget.rows.isEmpty && _table.emptyBuilder != null) _table.emptyBuilder!(context), - if (_table._paginated) _buildFooter(context), ], ); @@ -699,17 +725,35 @@ class _RemixDataTableViewState extends State<_RemixDataTableView> { return IntrinsicWidth( child: ConstrainedBox( constraints: BoxConstraints(minWidth: _table.minimumWidth), - child: content, + child: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + tableContent, + if (_table._paginated) _buildFooter(context), + ], + ), ), ); } - return SingleChildScrollView( - scrollDirection: Axis.horizontal, - child: SizedBox( - width: math.max(_table.minimumWidth, available), - child: content, - ), + return Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + SingleChildScrollView( + scrollDirection: Axis.horizontal, + child: SizedBox( + width: math.max(_table.minimumWidth, available), + child: tableContent, + ), + ), + // Deliberate: the footer is a sibling of the scroller, matching + // PaginatedDataTable — pagination stays visible while the table + // scrolls. Its top border spans the viewport, not the laid-out + // table width. + if (_table._paginated) _buildFooter(context), + ], ); }, ), diff --git a/packages/remix/test/components/data_table/data_table_style_test.dart b/packages/remix/test/components/data_table/data_table_style_test.dart index b058af04..e1e77362 100644 --- a/packages/remix/test/components/data_table/data_table_style_test.dart +++ b/packages/remix/test/components/data_table/data_table_style_test.dart @@ -51,6 +51,7 @@ List _boxColors(WidgetTester tester) { } Widget _table({ + List<_Record> rows = _records, DataTableStyler? style, DataTableSpec? styleSpec, bool selectable = false, @@ -59,7 +60,7 @@ Widget _table({ List>? columns, }) { return RemixDataTable<_Record>( - rows: _records, + rows: rows, columns: columns ?? _columns(), minimumWidth: minimumWidth, rowId: selectable ? (row) => row.id : null, @@ -142,6 +143,36 @@ void main() { expect(tester.takeException(), isNull); }); + testWidgets( + 'keeps pagination controls in the viewport while table scrolls', + (tester) async { + const viewportKey = ValueKey('data-table-viewport'); + await tester.pumpRemixApp( + SizedBox( + key: viewportKey, + width: 500, + child: RemixDataTable<_Record>( + rows: _records, + columns: _columns(), + minimumWidth: 640, + totalRows: 42, + onPageChanged: (_) {}, + onPageSizeChanged: (_) {}, + ), + ), + ); + + expect(tester.getSize(_tableFinder).width, 640); + final viewport = tester.getRect(find.byKey(viewportKey)); + expect( + tester + .getRect(find.byKey(const ValueKey('remix-data-table-next-page'))) + .right, + lessThanOrEqualTo(viewport.right), + ); + }, + ); + testWidgets('resolves flex columns under an unbounded parent', ( tester, ) async { @@ -327,6 +358,78 @@ void main() { expect(colors.where((c) => c == const Color(0xFF777777)), hasLength(2)); }); + testWidgets('provider-inherited stylers resolve on a hovered row', ( + tester, + ) async { + await tester.pumpRemixApp( + StyleProvider( + style: DataTableStyler().bodyRow( + BoxStyler() + .color(const Color(0xFF777777)) + .onHovered(BoxStyler().color(const Color(0xFF999999))), + ), + child: SizedBox(width: 400, child: _table()), + ), + ); + + final pointer = TestPointer(1, PointerDeviceKind.mouse); + await tester.sendEventToBinding( + pointer.hover(tester.getCenter(find.text('Ada'))), + ); + await tester.pump(); + + final colors = _boxColors(tester); + expect(colors.where((c) => c == const Color(0xFF999999)), hasLength(2)); + expect(colors.where((c) => c == const Color(0xFF777777)), hasLength(2)); + }); + + testWidgets('clears a hovered row index when row content changes', ( + tester, + ) async { + final style = DataTableStyler().bodyRow( + BoxStyler() + .color(const Color(0xFF777777)) + .onHovered(BoxStyler().color(const Color(0xFF999999))), + ); + final c = _Record('c', 'Curie'); + + await tester.pumpRemixApp( + SizedBox(width: 400, child: _table(style: style)), + ); + + final pointer = TestPointer(1, PointerDeviceKind.mouse); + await tester.sendEventToBinding( + pointer.hover(tester.getCenter(find.text('Blaise'))), + ); + await tester.pump(); + expect( + _boxColors(tester).where((c) => c == const Color(0xFF999999)), + hasLength(2), + ); + + await tester.pumpRemixApp( + SizedBox( + width: 400, + child: _table(rows: [_records.first], style: style), + ), + ); + // Once the hovered MouseRegion is gone, moving the pointer cannot fire + // its onExit callback. + await tester.sendEventToBinding(pointer.hover(const Offset(1000, 1000))); + await tester.pump(); + await tester.pumpRemixApp( + SizedBox( + width: 400, + child: _table(rows: [_records.first, c], style: style), + ), + ); + + expect( + _boxColors(tester).where((c) => c == const Color(0xFF999999)), + isEmpty, + ); + }); + testWidgets('selection visuals do not change row geometry', (tester) async { final style = DataTableStyler() .bodyRow( diff --git a/packages/remix/test/components/data_table/data_table_widget_test.dart b/packages/remix/test/components/data_table/data_table_widget_test.dart index 58f73297..ce3f2ec2 100644 --- a/packages/remix/test/components/data_table/data_table_widget_test.dart +++ b/packages/remix/test/components/data_table/data_table_widget_test.dart @@ -594,6 +594,31 @@ void main() { expect(find.byType(RemixIconButton), findsNothing); }); + testWidgets('rejects a page with more rows than its page size', ( + tester, + ) async { + await tester.pumpRemixApp( + RemixDataTable<_Record>( + rows: _records, + columns: _columns(), + totalRows: 3, + pageSize: 2, + pageSizeOptions: const [2], + onPageChanged: (_) {}, + onPageSizeChanged: (_) {}, + ), + ); + + expect( + tester.takeException(), + isA().having( + (error) => error.message, + 'message', + contains('exceeds pageSize'), + ), + ); + }); + testWidgets('keeps pagination outside the structural table node', ( tester, ) async {