feat: animate filter list transitionsFeat animate transitions#42
feat: animate filter list transitionsFeat animate transitions#42itsmeaabhii wants to merge 2 commits intochimon2000:masterfrom
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
There was a problem hiding this comment.
Pull request overview
This PR implements animated transitions when filtering the issue list, addressing issue #4. The changes replace the InitialCard widget with EmptyCard for all loading/empty states and wrap the issue list in an AnimatedSwitcher to provide smooth fade-in and slide-up animations (300ms) when the organization filter changes.
Changes:
- Added
AnimatedSwitcherwith fade and slide transitions toHomePagefor smooth state changes - Removed
InitialCardwidget entirely, consolidating to useEmptyCardfor loading and empty states - Updated test expectations to use
pumpAndSettle()to properly handle animations during testing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/ui/pages/home.dart | Added AnimatedSwitcher with fade+slide transition around issue list, added ValueKeys to trigger animations on state changes |
| lib/ui/widgets/widgets.dart | Removed export of deleted InitialCard widget |
| lib/ui/widgets/initial_card.dart | Deleted InitialCard widget (functionality replaced by EmptyCard) |
| test/pages/home_test.dart | Updated tests to expect EmptyCard instead of InitialCard and use pumpAndSettle() for animations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data: (state) => state.issues.isEmpty | ||
| ? 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 the ValueKey constructor causes unnecessary provider reads on every build. The current organization is already being watched at line 41, so this creates redundant provider subscriptions. Consider capturing the organization value in a local variable at the start of the build method and reusing it throughout, or use the organization value that's already available from the issueStoreProvider argument.
| // Verify that loading displays. | ||
| expect(find.byType(InitialCard), findsOneWidget); | ||
| expect(find.byType(EmptyCard), findsOneWidget); |
There was a problem hiding this comment.
The comment says "Verify that loading displays" but the test is now checking for EmptyCard. Since the loading state now shows EmptyCard instead of InitialCard (as per the code changes), this comment is misleading. Consider updating it to "Verify that empty card displays during loading" or similar to accurately reflect what's being tested.
| // Verify that loading displays. | ||
| expect(find.byType(InitialCard), findsOneWidget); | ||
| expect(find.byType(EmptyCard), findsOneWidget); |
There was a problem hiding this comment.
The comment says "Verify that loading displays" but the test is now checking for EmptyCard. Since the loading state now shows EmptyCard instead of InitialCard (as per the code changes), this comment is misleading. Consider updating it to "Verify that empty card displays during loading" or similar to accurately reflect what's being tested.
| @@ -115,9 +115,9 @@ void main() { | |||
| ); | |||
|
|
|||
| // Verify that loading displays. | |||
There was a problem hiding this comment.
The comment says "Verify that loading displays" but the test is now checking for EmptyCard. Since the loading state now shows EmptyCard instead of InitialCard (as per the code changes), this comment is misleading. Consider updating it to "Verify that empty card displays during loading" or similar to accurately reflect what's being tested.
| // Verify that loading displays. | |
| // Verify that empty card displays during loading. |
| child: queryResultState.when( | ||
| loading: () => const EmptyCard(key: ValueKey('loading')), | ||
| data: (state) => state.issues.isEmpty | ||
| ? EmptyCard(key: ValueKey('empty-${ref.watch(currentOrganizationProvider)}')) |
There was a problem hiding this comment.
Using ref.watch inside the ValueKey constructor causes unnecessary provider reads on every build. The current organization is already being watched at line 41, so this creates redundant provider subscriptions. Consider capturing the organization value in a local variable at the start of the build method and reusing it throughout, or use the organization value that's already available from the issueStoreProvider argument.
Fixes #4
Changes
AnimatedSwitcherwrapper around issue list inHomePageValueKeys based on state to trigger animations properlypumpAndSettle()Animation Details
Testing
Demo
[Add a screen recording showing the animation if possible]