Fix: Button padding and search feature for box origin (KIDS-1569) #1237
Conversation
WalkthroughThe changes made in this pull request primarily involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
lib/features/family/features/box_origin/box_origin_selection_page.dart(1 hunks)lib/features/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart(6 hunks)
🧰 Additional context used
📓 Learnings (1)
lib/features/family/features/box_origin/box_origin_selection_page.dart (1)
Learnt from: Daniela510
PR: givtnl/givt-app#1234
File: lib/features/family/features/box_origin/box_origin_selection_page.dart:60-98
Timestamp: 2024-11-05T17:30:27.414Z
Learning: In the `BoxOriginSelectionPage` (`lib/features/family/features/box_origin/box_origin_selection_page.dart`), error handling and state management are handled within the cubit's methods, so adding try-catch blocks in the UI layer is unnecessary.
🔇 Additional comments (6)
lib/features/family/features/box_origin/box_origin_selection_page.dart (2)
Line range hint 43-52: LGTM! Improved naming from 'fab' to 'button'.
The button configuration is properly maintained with all necessary properties:
- Disabled state tied to selection
- Loading state
- Analytics tracking
- Clear action handler
Line range hint 48-52: Analytics implementation aligns with PR objectives.
The code successfully implements behavior tracking using Amplitude as required:
- Tracks button clicks with
boxOriginConfirmClicked - Tracks selections with
boxOriginSelectedincluding relevant properties (namespace and organization name)
Also applies to: 61-67
lib/features/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart (4)
23-23: LGTM: Property renaming improves clarity
The renaming from fab to button better reflects its purpose and maintains consistency across components.
Also applies to: 29-29
38-38: LGTM: Selection state management
The selectedIndex state variable properly manages the selected item state.
Line range hint 59-82: LGTM: UI improvements enhance consistency
The changes improve the UI by:
- Using FunScaffold for consistent styling
- Explicit dependency injection with getIt
- Better button placement at the bottom
Also applies to: 94-96, 125-126
138-138: LGTM: Selection styling implementation
The selection styling is well-implemented with:
- Clear visual feedback using selectedTileColor
- Proper type-based color handling
Also applies to: 144-146
lib/features/family/features/box_origin/box_origin_selection_page.dart
Outdated
Show resolved
Hide resolved
...res/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart
Outdated
Show resolved
Hide resolved
...res/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
lib/features/family/shared/design/components/content/fun_organisation_filter_tile_bar.dart (1)
41-43: Improve code readability with explicit variablesThe ternary operation could be more readable with explicit variable names to clarify the toggle behavior.
Consider this refactoring:
-state.selectedType == e.index - ? CollectGroupType.none.index - : e.index, +final isAlreadySelected = state.selectedType == e.index; +final newTypeIndex = isAlreadySelected + ? CollectGroupType.none.index + : e.index; +newTypeIndex,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
lib/features/family/features/box_origin/box_origin_selection_page.dart(1 hunks)lib/features/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart(6 hunks)lib/features/family/shared/design/components/content/fun_organisation_filter_tile_bar.dart(2 hunks)lib/shared/bloc/organisation/organisation_bloc.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/family/features/box_origin/box_origin_selection_page.dart
🔇 Additional comments (6)
lib/features/family/shared/design/components/content/fun_organisation_filter_tile_bar.dart (1)
3-3: LGTM: Import addition is appropriate
The new import for CollectGroupType is necessary for the type deselection functionality.
lib/features/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart (4)
23-29: LGTM! Property changes improve flexibility and clarity.
The renaming of fab to button and making onTap nullable enhances the component's reusability and clarity.
94-96: LGTM! Good use of dependency injection.
The change to use getIt for accessing the OrganisationBloc is a good practice.
139-147: LGTM! Good visual feedback implementation.
The selection state and type-specific colors provide clear visual feedback to users.
43-46:
Add analytics tracking for organization fetching.
As per PR objectives, Amplitude tracking should be implemented. Consider adding analytics when organizations are fetched.
Add this after the OrganisationFetch event:
getIt<OrganisationBloc>().add(
OrganisationFetch(
Country.fromCode(context.read<AuthCubit>().state.user.country),
type: CollectGroupType.none.index,
),
);
+getIt<AmplitudeAnalytics>().logEvent(
+ 'organization_list_viewed',
+ eventProperties: {
+ 'country': context.read<AuthCubit>().state.user.country,
+ 'type': CollectGroupType.none.name,
+ },
+);Likely invalid or redundant comment.
lib/shared/bloc/organisation/organisation_bloc.dart (1)
202-206: LGTM! Improved type filtering logic.
The conditional check ensures that organizations are only filtered by type when a specific type is selected, allowing for proper clearing of type filters.
Let's verify the integration with UI components:
✅ Verification successful
Based on the shell script results, I can now generate the final response:
✅ Type filtering implementation is correctly integrated with UI components
The code changes are well integrated across the application:
fun_organisation_filter_tile_bar.dartcorrectly toggles between selected type andCollectGroupType.none.indexorganization_list_page.dartproperly handles all organization types and none state- The bloc's filtering logic aligns with UI state management needs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for UI components dispatching OrganisationTypeChanged events
# and handling CollectGroupType.none.index
# Search for UI components dispatching OrganisationTypeChanged events
rg "OrganisationTypeChanged" -A 3
# Search for usages of CollectGroupType.none
rg "CollectGroupType\.none" -A 3
Length of output: 10650
lib/features/family/shared/design/components/content/fun_organisation_filter_tile_bar.dart
Outdated
Show resolved
Hide resolved
...res/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart (1)
Line range hint
42-54: Add technical logging for error states.While the error is shown to the user via SnackBar, we should also log it through Logit for debugging purposes, as mentioned in the PR objectives.
if (state.status == GiveStatus.error) { + final error = state.error ?? 'Unknown error'; + LogitHelper.error( + 'Error in GiveFromListPage', + error: error, + properties: {'status': state.status}, + ); ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text(locals.somethingWentWrong), ), ); }lib/features/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart (3)
22-23: Consider renamingonTapListItemfor clarity.The callback name could be more descriptive to indicate its purpose. Consider renaming it to
onOrganisationSelectedoronCollectGroupSelectedto better reflect its functionality.- final void Function(CollectGroup)? onTapListItem; + final void Function(CollectGroup)? onOrganisationSelected;Also applies to: 30-31
95-95: Fix typo in property name.There's a typo in the property name
stratPadding. It should bestartPadding.- stratPadding: 0, + startPadding: 0,
Line range hint
73-82: Enhance error handling in BlocConsumer.The current error handling only shows a generic error message. Consider:
- Adding different error messages based on the error type
- Adding a retry action
if (state.status == OrganisationStatus.error) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( - content: Text(locals.somethingWentWrong), + content: Row( + children: [ + Expanded( + child: Text(state.error?.message ?? locals.somethingWentWrong), + ), + TextButton( + onPressed: () => getIt<OrganisationBloc>().add( + OrganisationFetch( + Country.fromCode( + context.read<AuthCubit>().state.user.country, + ), + type: CollectGroupType.none.index, + ), + ), + child: Text(locals.retry), + ), + ], + ), ), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
lib/features/family/features/box_origin/box_origin_selection_page.dart(2 hunks)lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart(1 hunks)lib/features/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart(6 hunks)lib/features/family/shared/design/components/content/fun_organisation_filter_tile_bar.dart(2 hunks)
🧰 Additional context used
📓 Learnings (1)
lib/features/family/features/box_origin/box_origin_selection_page.dart (1)
Learnt from: Daniela510
PR: givtnl/givt-app#1234
File: lib/features/family/features/box_origin/box_origin_selection_page.dart:60-98
Timestamp: 2024-11-05T17:30:27.414Z
Learning: In the `BoxOriginSelectionPage` (`lib/features/family/features/box_origin/box_origin_selection_page.dart`), error handling and state management are handled within the cubit's methods, so adding try-catch blocks in the UI layer is unnecessary.
🔇 Additional comments (7)
lib/features/family/shared/design/components/content/fun_organisation_filter_tile_bar.dart (5)
3-3: LGTM: Import statement is correctly placed and necessary.
9-14: LGTM: Constructor is well-structured with proper parameter organization.
17-17: LGTM: Callback property is properly typed and documented.
41-52: LGTM: Filter tile implementation is logically sound.
The type switching logic and state management are implemented correctly.
44-52: Add analytics tracking for filter interactions.
The PR objectives mention implementing behavior tracking using Amplitude, but the filter interactions are not being tracked.
Note: This issue was previously identified in an earlier review. Please refer to the existing comment for the suggested implementation.
lib/features/family/features/box_origin/box_origin_selection_page.dart (1)
51-57: LGTM! Analytics implementation aligns with requirements.
The Amplitude event tracking is well-implemented with appropriate event properties, fulfilling the PR objective of implementing behavior tracking.
lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart (1)
Line range hint 74-103: LGTM: Analytics tracking is properly implemented.
The analytics tracking for the parent giving flow is well implemented using Amplitude events, capturing relevant properties such as organization name, amount, and medium ID.
lib/features/family/features/box_origin/box_origin_selection_page.dart
Outdated
Show resolved
Hide resolved
lib/features/family/features/parent_giving_flow/presentation/pages/give_from_list_page.dart
Show resolved
Hide resolved
...res/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart
Show resolved
Hide resolved
...res/family/features/parent_giving_flow/presentation/pages/organisation_list_family_page.dart
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
buttonTextfor clarity.