Conversation
…idgets - Removed InitialCard widget (duplicate of EmptyCard) - Updated widgets barrel export to remove InitialCard - Updated home.dart to use EmptyCard for both loading and empty states - Updated tests to reflect consolidated widget usage Fixes chimon2000#37"
- Added AnimatedSwitcher to home page for smooth transitions - Implemented fade + slide animation (300ms) when filter changes - Added unique ValueKeys for proper animation detection - Updated tests to use pumpAndSettle for animation completion Fixes chimon2000#4
- Convert AboutPage from StatelessWidget to ConsumerWidget - Replace direct url_launcher calls with LinkService via Riverpod - Remove duplicate URL launching implementation (lines 76-81) - Add flutter_riverpod and app_providers imports - Ensure consistent URL launching behavior across all pages Fixes chimon2000#35
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #35 by consolidating duplicate URL launching logic in AboutPage to use the centralized LinkService through Riverpod dependency injection. However, the PR also includes significant, undocumented changes to the home page's loading state handling and animations.
Changes:
- Converted AboutPage from StatelessWidget to ConsumerWidget and replaced direct
url_launchercalls withLinkServicevia Riverpod - Replaced InitialCard with EmptyCard throughout the app and removed the InitialCard widget entirely
- Added AnimatedSwitcher with fade and slide transitions to the home page's content area
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/ui/pages/about.dart | Converted to ConsumerWidget and replaced direct url_launcher calls with LinkService via linkServiceProvider |
| lib/ui/pages/home.dart | Replaced InitialCard with EmptyCard, added AnimatedSwitcher with transitions, and added ValueKey to child widgets |
| lib/ui/widgets/widgets.dart | Removed export of initial_card.dart |
| lib/ui/widgets/initial_card.dart | Deleted the InitialCard widget (replaced by EmptyCard) |
| test/pages/home_test.dart | Updated test expectations from InitialCard to EmptyCard and changed pump() to pumpAndSettle() to handle animations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| child: AnimatedSwitcher( | ||
| duration: const Duration(milliseconds: 300), | ||
| switchInCurve: Curves.easeInOut, | ||
| switchOutCurve: Curves.easeInOut, | ||
| transitionBuilder: (child, animation) { | ||
| return FadeTransition( | ||
| opacity: animation, | ||
| child: SlideTransition( | ||
| position: Tween<Offset>( | ||
| begin: const Offset(0.0, 0.1), | ||
| end: Offset.zero, | ||
| ).animate(animation), | ||
| child: child, | ||
| ), | ||
| ); | ||
| }, | ||
| child: queryResultState.when( | ||
| loading: () => const EmptyCard(key: ValueKey('loading')), | ||
| data: (state) => state.issues.isEmpty | ||
| ? EmptyCard(key: ValueKey('empty-${ref.watch(currentOrganizationProvider)}')) | ||
| : IssueListDataView( | ||
| key: ValueKey('data-${ref.watch(currentOrganizationProvider)}-${state.issues.length}'), | ||
| issuesQueryResult: state, | ||
| ), | ||
| error: (error, _) => Text( | ||
| error.toString(), | ||
| key: ValueKey('error-$error'), | ||
| ), | ||
| ), |
There was a problem hiding this comment.
The PR description states this PR addresses issue #35 by consolidating duplicate URL launching logic in AboutPage, but the changes to home.dart (replacing InitialCard with EmptyCard and adding AnimatedSwitcher) are not mentioned in the description. These changes appear to be unrelated to the URL launching consolidation work described. Consider either updating the PR description to explain these additional changes, or splitting them into a separate PR if they address a different issue.
| ? EmptyCard(key: ValueKey('empty-${ref.watch(currentOrganizationProvider)}')) | ||
| : IssueListDataView( | ||
| key: ValueKey('data-${ref.watch(currentOrganizationProvider)}-${state.issues.length}'), |
There was a problem hiding this comment.
Using ref.watch inside ValueKey constructors (lines 93 and 95) can cause unnecessary widget rebuilds. When currentOrganizationProvider changes, the entire widget tree will rebuild, and these ref.watch calls will execute again during the build phase. Since the organization value is already available in the surrounding scope or can be accessed via ref.read, consider using ref.read instead to avoid redundant provider subscriptions within the key construction.
Fixed ✅
This issue has been resolved by consolidating the duplicate URL launching logic in AboutPage.
Changes Made
url_launchercalls withLinkServiceviaref.read(linkServiceProvider)lib/ui/pages/about.dart)flutter_riverpodandapp_providersinstead of directurl_launcherResults
✅ All 12 tests passing
✅ No compilation errors
✅ Consistent URL launching behavior across all pages (AboutPage, IssueDetailPage, MorePage)
✅ Single source of truth via LinkService
✅ Follows project's Riverpod state management conventions
The AboutPage now properly uses the existing
LinkServicethrough Riverpod providers, eliminating code duplication and ensuring maintainability.Commit
The fix has been committed with message: