feat: added customizable page indicator widget - #4
Conversation
|
cool i will review when i get time |
|
Hi! Just a reminder about this PR whenever you have some free time. I know you're probably busy, but I'd appreciate a review when it's convenient. Thanks! 🙂 |
hi yes, i will review it this weekend |
I reviewed the code. Great work but there are some issues that should be fixed -
|
There was a problem hiding this comment.
Pull request overview
This PR adds a reusable CoverflowPageIndicator widget to the package so apps can display (and optionally tap) a page indicator driven by CoverflowCarouselController.pageListenable, and updates the example and tests accordingly.
Changes:
- Added
CoverflowPageIndicatorwidget (exported from the package entrypoint). - Updated the example app to use the new public widget and added UI controls for indicator styling.
- Added widget tests for rendering and tap behavior.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/coverflow_page_indicator.dart | Introduces the new indicator widget that listens to the controller and renders tappable dots + an active “pill”. |
| lib/coverflow_carousel.dart | Exports the new widget as part of the public API. |
| example/lib/main.dart | Replaces the example’s private indicator implementation with the new public CoverflowPageIndicator and adds customization controls. |
| test/coverflow_page_indicator_test.dart | Adds tests covering basic rendering and tap callback behavior. |
| example/pubspec.lock | Updates the example lockfile to the new package version reference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (7)
lib/src/coverflow_page_indicator.dart:32
- The indicators are given a 40x40 tap target, but they’re positioned with negative offsets; since the parent Stack/SizedBox bounds are smaller, a large portion of the intended tap target will be outside the render box and won’t receive pointer events. Compute a non-negative padding and use it to size/position the tap targets within the widget’s bounds.
final step = dotSize + dotSpacing;
lib/src/coverflow_page_indicator.dart:48
- The widget’s width/height don’t account for the larger tap targets, so hit testing will be clipped to a much smaller area than intended. Expand the widget bounds to include the computed tap padding and ensure height is at least the tap target size.
return SizedBox(
width: count * dotSize + (count - 1) * dotSpacing,
height: dotSize + 8,
test/coverflow_page_indicator_test.dart:95
find.byType(SizedBox)is too broad here (MaterialApp/Scaffold can include other SizedBoxes), which can make this test flaky. Narrow the assertion to the SizedBox returned by CoverflowPageIndicator.
expect(find.byType(SizedBox), findsOneWidget);
lib/src/coverflow_page_indicator.dart:57
- These tap targets are positioned with negative offsets (and can end up outside the Stack’s hit-test bounds). Position them within the Stack (non-negative) and use the separate
padoffset for the visual dot/pill alignment instead.
left: i * step - (_tapTargetSize - dotSize) / 2,
top: 4 - (_tapTargetSize - dotSize) / 2,
width: _tapTargetSize,
height: _tapTargetSize,
lib/src/coverflow_page_indicator.dart:75
- Once the dots are padded to keep their 40px tap targets in-bounds, the active pill needs the same horizontal padding; otherwise it won’t align with the dots.
_buildActivePill(
lib/src/coverflow_page_indicator.dart:86
- The non-wrapping active pill positioning also needs the same
padoffset so it stays aligned with the (padded) dots.
(t < 0.5
? (t / 0.5) * step
: (1.0 - (t - 0.5) / 0.5) * step),
lib/src/coverflow_page_indicator.dart:104
top: 4hard-codes vertical placement of the active pill. If the indicator height is increased to fit the 40px tap targets, the pill should be vertically centered using the same padding calculation as the dots so it stays aligned.
color: activeColor,
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (9)
lib/src/coverflow_page_indicator.dart:44
clampedalways wraps with modulo, which can make a non-infinite carousel show a wrap-around animation (last → first) during iOS-style overscroll, becauseCoverflowCarouselController.pageListenableis only normalized for infinite carousels (see controller docs +CoverflowCarousel’supdateMetricscall). Consider adding an explicitisInfinite/wrapAroundparameter (defaulting tofalse) so finite carousels clamp instead of wrap.
final count = itemCount;
final clamped = page.clamp(0.0, count.toDouble() - 1e-9).toDouble();
final floor = clamped.floor();
final t = clamped - floor;
final indexB = (floor + 1) % count;
final isWrapping = count > 1 && indexB == 0 && floor == count - 1;
example/lib/main.dart:313
_indicatorDotSizeis user-adjustable in the settings panel, but it isn’t passed intoCoverflowPageIndicator, so the “Indicator Dot Size” slider currently has no effect.
CoverflowPageIndicator(
controller: _controller,
itemCount: _demoCards.length,
activeColor: _indicatorUseThemeColor
? Theme.of(context).colorScheme.primary
lib/src/coverflow_page_indicator.dart:50
- The
_tapTargetSizehit areas are positioned with negativeleft/topoffsets while the overall widget is onlydotSize + 8tall and exactlyitemCountdots wide. Because Flutter hit-testing is clipped to the parent’s bounds, taps near the first/last dot (and above/below the indicator) can fall outside the widget bounds and won’t register.
return SizedBox(
width: count * dotSize + (count - 1) * dotSpacing,
height: dotSize + 8,
child: Stack(
clipBehavior: Clip.none,
test/coverflow_page_indicator_test.dart:8
- The test creates a
CoverflowCarouselControllerbut never disposes it. Since the controller owns StreamControllers and ValueNotifiers, it should be disposed to avoid resource leaks across the test suite.
final controller = CoverflowCarouselController();
test/coverflow_page_indicator_test.dart:22
- The test creates a
CoverflowCarouselControllerbut never disposes it. Since the controller owns StreamControllers and ValueNotifiers, it should be disposed to avoid resource leaks across the test suite.
final controller = CoverflowCarouselController();
test/coverflow_page_indicator_test.dart:40
- The test creates a
CoverflowCarouselControllerbut never disposes it. Since the controller owns StreamControllers and ValueNotifiers, it should be disposed to avoid resource leaks across the test suite.
final controller = CoverflowCarouselController();
test/coverflow_page_indicator_test.dart:63
- The test creates a
CoverflowCarouselControllerbut never disposes it. Since the controller owns StreamControllers and ValueNotifiers, it should be disposed to avoid resource leaks across the test suite.
final controller = CoverflowCarouselController();
test/coverflow_page_indicator_test.dart:85
- The test creates a
CoverflowCarouselControllerbut never disposes it. Since the controller owns StreamControllers and ValueNotifiers, it should be disposed to avoid resource leaks across the test suite.
final controller = CoverflowCarouselController();
test/coverflow_page_indicator_test.dart:100
- The test creates a
CoverflowCarouselControllerbut never disposes it. Since the controller owns StreamControllers and ValueNotifiers, it should be disposed to avoid resource leaks across the test suite.
final controller = CoverflowCarouselController();
GalaxyPhoenix716
left a comment
There was a problem hiding this comment.
fix the errors and resolve the suggestions given by copilot
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
example/lib/main.dart:316
- The example exposes an “Indicator Dot Size” slider via
_indicatorDotSize, but the value is never passed toCoverflowPageIndicator, so the control has no effect.
activeColor: _indicatorUseThemeColor
? Theme.of(context).colorScheme.primary
: Colors.white,
dotSpacing: _indicatorDotSpacing,
onTap: (index) {
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
lib/src/coverflow_page_indicator.dart:50
- The dot tap targets are positioned outside the current SizedBox bounds (negative left/top), but RenderBox hit-testing is clipped to the parent size. This makes the intended 40×40 tap targets partially/non-tappable (especially first/last dots and vertically), which is an accessibility/usability bug.
return SizedBox(
width: count * dotSize + (count - 1) * dotSpacing,
height: dotSize + 8,
child: Stack(
clipBehavior: Clip.none,
lib/src/coverflow_page_indicator.dart:100
- After making the tap targets fully in-bounds, the active pill needs the same offset as the dot centers; otherwise it will render misaligned relative to the dots.
Widget _buildActivePill({required double left, required double width}) {
return Positioned(
left: left,
top: 4,
width: width,
height: dotSize,
child: IgnorePointer(
example/lib/main.dart:316
- The example adds an “Indicator Dot Size” control (_indicatorDotSize) but doesn’t pass it to CoverflowPageIndicator, so the slider has no effect.
CoverflowPageIndicator(
controller: _controller,
itemCount: _demoCards.length,
activeColor: _indicatorUseThemeColor
? Theme.of(context).colorScheme.primary
: Colors.white,
dotSpacing: _indicatorDotSpacing,
onTap: (index) {
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
Added a customizable CoverflowPageIndicator widget.
Changes
Fixes #2