Feat: Added ShadPagination. Implementation of #237 #581
Conversation
|
Warning Rate limit exceeded@asare-21 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 6 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds a new ShadPagination component with ShadPaginationController, pagination theming (ShadPaginationTheme and generated mixin), exports the component, playground example and route, and integrates pagination theme into ShadThemeData and theme variants. Changes
Sequence DiagramsequenceDiagram
participant User
participant Widget as ShadPagination
participant Controller as ShadPaginationController
participant Theme as ShadTheme
participant Callback as onPageChanged
User->>Widget: instantiate (totalPages, controller?)
alt controller provided
Widget->>Controller: use provided controller
else
Widget->>Controller: create internal controller (init initialPage)
end
Widget->>Theme: resolve paginationTheme
Widget->>Controller: listen (ListenableBuilder)
User->>Widget: clicks page / nav button
Widget->>Controller: set selectedIndex(newIndex)
Controller->>Widget: notifyListeners (rebuild)
Widget->>Theme: apply styling
Widget->>Callback: call onPageChanged(newIndex)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
lib/src/components/pagination.dart (1)
513-514: Consider using dart:math for min/max functions.The widget defines local
minandmaxhelper methods. Dart'sdart:mathlibrary provides these as top-level functions. Using the standard library functions would be more idiomatic and eliminate the need for custom implementations.🔎 Proposed change
+import 'dart:math' show max, min; import 'package:flutter/material.dart'; import 'package:shadcn_ui/shadcn_ui.dart'; // ... rest of file ... - T min<T extends num>(T a, T b) => a < b ? a : b; - T max<T extends num>(T a, T b) => a > b ? a : b; }playground/lib/pages/pagination.dart (3)
9-9: Addconstkeyword for consistency.
PaginationExamplehas a const constructor, so the instantiation should useconst.🔎 Proposed fix
@override Widget build(BuildContext context) { - return PaginationExample(); + return const PaginationExample(); }
40-40: Remove or update the stale comment.The comment "Add your pagination widget implementation here" is misleading since the implementation is already present on Line 41.
🔎 Proposed fix
const SizedBox(height: 20), - // Add your pagination widget implementation here ShadPagination(
45-47: Consider usingdebugPrintinstead ofWhile
debugPrintis the Flutter-recommended alternative as it handles longer outputs and is automatically stripped in release builds.🔎 Proposed fix
onPageChanged: (value) { - print('Page changed to ${value + 1}'); + debugPrint('Page changed to ${value + 1}'); },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.mdlib/shadcn_ui.dartlib/src/components/pagination.dartlib/src/theme/components/pagination.dartlib/src/theme/components/pagination.g.theme.dartlib/src/theme/data.dartlib/src/theme/themes/base.dartlib/src/theme/themes/default_theme_no_secondary_border_variant.dartlib/src/theme/themes/default_theme_variant.dartplayground/.gitignoreplayground/.metadataplayground/lib/pages/pagination.dartplayground/lib/router.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-13T09:57:53.516Z
Learnt from: nank1ro
Repo: nank1ro/flutter-shadcn-ui PR: 352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
Applied to files:
lib/src/theme/themes/default_theme_no_secondary_border_variant.dartlib/src/theme/themes/base.dartlib/src/theme/data.dartlib/src/theme/components/pagination.dartlib/src/theme/themes/default_theme_variant.dartlib/src/theme/components/pagination.g.theme.dartlib/src/components/pagination.dart
🔇 Additional comments (10)
lib/src/theme/data.dart (3)
23-23: LGTM!The import is correctly placed in alphabetical order among the component imports.
113-113: LGTM!The pagination theme integration follows the established pattern: accepting an optional
paginationThemeparameter in the factory constructor and merging it with the variant's default viaeffectiveVariant.paginationTheme().merge(paginationTheme). This is consistent with all other theme components in this file.Also applies to: 245-248
307-307: LGTM!The internal constructor correctly propagates
shadPaginationThemeas a required super parameter.lib/src/theme/themes/base.dart (3)
22-22: LGTM!Import correctly placed in alphabetical order.
99-99: LGTM!The
shadPaginationThemefield and constructor parameter are correctly integrated intoShadBaseTheme, following the established pattern for theme components.Also applies to: 153-153
210-210: LGTM!The
paginationTheme()abstract method is correctly added toShadThemeVariant, enabling theme variants to provide their pagination theme configuration.playground/lib/pages/pagination.dart (1)
20-27: LGTM!The controller lifecycle is properly managed—initialized as a field and disposed in
dispose()with the correct call order (_controller.dispose()beforesuper.dispose()).lib/src/theme/components/pagination.g.theme.dart (1)
1-399: Generated file - no review required.This is auto-generated code from
theme_extensions_builder_annotation. The implementation correctly handles lerp, copyWith, merge, equality, and hashCode for all theme properties.lib/src/theme/components/pagination.dart (2)
10-53: LGTM!The
ShadPaginationThemeclass follows the established pattern for theme components in this package:
- Uses
@themeGenand@immutableannotations- Mixes in the generated
_$ShadPaginationTheme- Implements the
canMergepattern with private backing field- Provides sensible defaults for non-nullable properties
The comprehensive set of styling properties enables flexible customization of the pagination component.
175-179: LGTM!The static
lerpmethod correctly delegates to the generated mixin implementation, following the established pattern.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/src/components/pagination.dart (2)
501-509: Consider using a Set to track added pages.The loop uses
pages.contains(i), which is O(n) per iteration, making this section O(n²). For typical pagination with small page counts, the performance impact is negligible. However, for better algorithmic complexity, consider tracking added page numbers in aSet<int>and checking membership there.🔎 Potential optimization
List<int?> _generatePageNumbers() { if (widget.totalPages <= 0) return []; final currentPage = controller.selectedIndex + 1; // Convert to 1-based final totalPages = widget.totalPages; final siblingCount = widget.siblingCount; final boundaryCount = widget.boundaryCount; final pages = <int?>[]; + final addedPages = <int>{}; // For small number of pages, show all if (totalPages <= (boundaryCount * 2 + siblingCount * 2 + 3)) { for (var i = 1; i <= totalPages; i++) { pages.add(i); } return pages; } // Always show first boundary pages for (var i = 1; i <= min(boundaryCount, totalPages); i++) { pages.add(i); + addedPages.add(i); } // ... [middle sections would also update addedPages] // Always show last boundary pages for ( var i = max(totalPages - boundaryCount + 1, boundaryCount + 1); i <= totalPages; i++ ) { - if (i > boundaryCount && !pages.contains(i)) { + if (i > boundaryCount && !addedPages.contains(i)) { pages.add(i); + addedPages.add(i); } } return pages; }
654-656: Consider using dart:math for min/max functions.The custom
minandmaximplementations are correct, butdart:mathprovides built-in versions that would reduce code and improve maintainability.🔎 Proposed refactor
Add the import at the top of the file:
import 'package:flutter/material.dart'; import 'package:shadcn_ui/shadcn_ui.dart'; +import 'dart:math';Remove the custom implementations:
- T min<T extends num>(T a, T b) => a < b ? a : b; - T max<T extends num>(T a, T b) => a > b ? a : b;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/components/pagination.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-13T09:57:53.516Z
Learnt from: nank1ro
Repo: nank1ro/flutter-shadcn-ui PR: 352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
Applied to files:
lib/src/components/pagination.dart
🔇 Additional comments (3)
lib/src/components/pagination.dart (3)
83-98: Parameter validation implemented correctly.The assertions added here address the previous review comment requesting validation for pagination configuration parameters. All critical parameters (
totalPages,initialPage,siblingCount,boundaryCount,buttonHeight,compactBreakpoint) are now validated with clear error messages.
272-284: Controller initialization logic correctly addresses past feedback.The initialization now respects a provided controller's state while still supporting
initialPagewhen the controller is at its default. The logic only appliesinitialPagewhen the provided controller'sselectedIndexis 0 andinitialPageis non-zero, avoiding silent overwrites of explicit user state.
4-656: Excellent implementation of the pagination component.The
ShadPaginationwidget is well-architected with:
- Proper resource management (controller lifecycle)
- Comprehensive theming integration
- Responsive design with compact mode support
- Accessible UI (disabled states, tooltips)
- Flexible page number generation with ellipsis logic
- Extensive customization options
The implementation addresses all previously raised concerns and follows the package's established patterns.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
test/src/components/pagination.dart (1)
327-365: Consider resetting test view dimensions after modification.Tests at lines 327-344 and 346-364 modify
tester.view.physicalSizebut don't reset it afterward. While Flutter test framework typically handles cleanup, explicitly resetting the view ensures test isolation and prevents potential interference with subsequent tests.🔎 Suggested approach to reset view
Add an
addTearDowncall or reset the view at the end of each test:testWidgets('switches to compact mode below breakpoint', (tester) async { // Set screen width below default breakpoint (768) tester.view.physicalSize = const Size(500, 800); tester.view.devicePixelRatio = 1.0; + addTearDown(() { + tester.view.resetPhysicalSize(); + tester.view.resetDevicePixelRatio(); + }); await tester.pumpWidget( const MaterialApp( home: Scaffold( body: ShadPagination( totalPages: 10, ), ), ), ); // Should be in compact mode due to screen width expect(find.byType(ShadButton), findsAtLeast(3)); });Apply similar changes to the test at lines 346-364.
lib/src/components/pagination.dart (3)
278-283: Clarify the initialPage vs. external controller precedence.The logic attempts to apply
initialPageonly when the provided controller is at the default value (0), but this creates ambiguous behavior:
- If a user provides a controller at index 0 and wants to use
initialPage=0, the condition passes but nothing happens (correct by coincidence).- If a user provides a controller at index 3 and also passes
initialPage=5, theinitialPageis silently ignored (potentially surprising).- Index 0 is a valid page selection, so treating it as "uninitialized" is assumption-based.
Consider one of these clearer approaches:
- Always respect external controller (recommended): Document that
initialPageonly applies when no controller is provided.- Always apply initialPage: Document that
initialPageoverrides the controller's initial state.- Add an assertion: Fail fast if both a non-null controller and a non-zero
initialPageare provided together.🔎 Option 1 (recommended): Only use initialPage with internal controller
@override void initState() { super.initState(); if (widget.controller == null) { _controller = ShadPaginationController(); _controller.selectedIndex = widget.initialPage; - } else { - // Respect the provided controller's state, but apply initialPage - // if it's at default - if (widget.controller!.selectedIndex == 0 && widget.initialPage != 0) { - widget.controller!.selectedIndex = widget.initialPage; - } } + // When an external controller is provided, respect its state entirely + // and ignore initialPage }Update the
initialPagedocumentation to clarify:/// The initial page index. /// Only used when [controller] is null. If you provide an external /// controller, set its selectedIndex before passing it to ShadPagination. final int initialPage;🔎 Option 3: Fail fast with assertion
Add an assertion in the constructor:
const ShadPagination({ // ... parameters }) : assert(totalPages > 0, 'totalPages must be greater than 0'), // ... other assertions + assert( + controller == null || initialPage == 0, + 'Cannot specify both controller and non-zero initialPage. ' + 'Either provide a controller with pre-set selectedIndex, ' + 'or use initialPage without a controller.', + ), assert(boundaryCount >= 0, 'boundaryCount must be non-negative');
500-509: Consider simplifying the boundary page logic to avoid potential duplicates.The loop at lines 501-509 adds the last boundary pages, but it uses
pages.contains(i)to avoid duplicates. This check is O(n) for each iteration and suggests the logic might add pages that were already included. Using a Set or clearer range logic would be more efficient and clearer.Additionally, the loop range
max(totalPages - boundaryCount + 1, boundaryCount + 1)is complex and could have edge cases that are hard to reason about.🔎 Alternative approach using Set for deduplication
List<int?> _generatePageNumbers() { if (widget.totalPages <= 0) return []; final currentPage = controller.selectedIndex + 1; // Convert to 1-based final totalPages = widget.totalPages; final siblingCount = widget.siblingCount; final boundaryCount = widget.boundaryCount; + final pageSet = <int>{}; final pages = <int?>[]; // For small number of pages, show all if (totalPages <= (boundaryCount * 2 + siblingCount * 2 + 3)) { for (var i = 1; i <= totalPages; i++) { pages.add(i); } return pages; } // Collect all pages that should be shown (without ellipsis) // Add first boundary pages for (var i = 1; i <= min(boundaryCount, totalPages); i++) { - pages.add(i); + pageSet.add(i); } // Add current page and siblings final start = max(currentPage - siblingCount, 1); final end = min(currentPage + siblingCount, totalPages); for (var i = start; i <= end; i++) { - if (i > boundaryCount && i <= totalPages - boundaryCount) { - pages.add(i); - } + pageSet.add(i); } // Add last boundary pages for (var i = max(totalPages - boundaryCount + 1, 1); i <= totalPages; i++) { - if (i > boundaryCount && !pages.contains(i)) { - pages.add(i); - } + pageSet.add(i); } + // Convert set to list with ellipses + final sortedPages = pageSet.toList()..sort(); + for (var i = 0; i < sortedPages.length; i++) { + if (i > 0 && sortedPages[i] - sortedPages[i - 1] > 1 && widget.showEllipsis) { + pages.add(null); // Add ellipsis + } + pages.add(sortedPages[i]); + } - // Add ellipsis logic... return pages; }This approach is clearer: collect all pages in a Set (which handles duplicates automatically), sort them, then insert ellipses where there are gaps.
634-639: Consider disabling the selected page button.Lines 634-639 make the button's
onPressedcallback checkif (!isSelected)before updating, but the button itself is always pressable. This means users can tap the currently selected page, and nothing happens—which might feel unresponsive or buggy.Consider setting
onPressed: nullfor selected pages to visually indicate they're not actionable.🔎 Proposed change
return ShadButton.raw( variant: effectiveVariant, size: widget.buttonSize ?? ShadButtonSize.sm, padding: widget.buttonPadding ?? const EdgeInsets.symmetric(horizontal: 12, vertical: 6), backgroundColor: effectiveBackgroundColor, - onPressed: () { - if (!isSelected) { - controller.selectedIndex = page - 1; - widget.onPageChanged?.call(controller.selectedIndex); - } - }, + onPressed: isSelected + ? null + : () { + controller.selectedIndex = page - 1; + widget.onPageChanged?.call(controller.selectedIndex); + }, child: Container( height: widget.buttonHeight, alignment: Alignment.center, child: Text( page.toString(), style: TextStyle( fontSize: 14, color: isSelected ? effectiveForegroundColor : null, ), ), ), );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test/src/components/failures/input_masterImage.pngis excluded by!**/*.pngtest/src/components/failures/input_testImage.pngis excluded by!**/*.png
📒 Files selected for processing (2)
lib/src/components/pagination.darttest/src/components/pagination.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-05-13T09:57:53.516Z
Learnt from: nank1ro
Repo: nank1ro/flutter-shadcn-ui PR: 352
File: docs/src/content/docs/index.md:129-135
Timestamp: 2025-05-13T09:57:53.516Z
Learning: The shadcn_ui package uses `WidgetStatePropertyAll` in theme configurations (such as `ScrollbarThemeData`) instead of Flutter's native `MaterialStatePropertyAll`. This is intentional and correct according to the package maintainer.
Applied to files:
lib/src/components/pagination.dart
🔇 Additional comments (1)
lib/src/components/pagination.dart (1)
326-327: No issues found. TheEdgeInsetsGeometry.horizontalproperty is a standard Flutter API and works as expected. No changes needed.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This PR worked on #237. I have implemented the pagination widget and followed the contribution guidelines
Issues worked on: #237
Screenshot of the implementation
Testing this PR
To try this branch, add the following to your
pubspec.yaml:Pre-launch Checklist
///).0.18.0and you introduced a breaking change or a new feature, bump it to0.19.0, if you just added a fix or a chore bump it to0.18.1.CHANGELOG.mdfile with a summary of changes made following the format already used.If you need help, consider asking for advice on Discord.
Summary by CodeRabbit
New Features
Theme
Examples
Tests
Changelog
✏️ Tip: You can customize this high-level summary in your review settings.