Fix: Filter dialog confirmation button #2537
Conversation
WalkthroughFilterDialog now uses local state ( Changes
Sequence DiagramsequenceDiagram
participant User
participant Dialog as FilterDialog (UI)
participant State as LocalState\n(selectedFilter)
participant Parent as ParentComponent
User->>Dialog: Tap an option
Dialog->>State: set selectedFilter (option or null for "All")
Note over Dialog,State: Dialog stays open
User->>Dialog: Tap Confirm
Dialog->>State: read selectedFilter
Dialog->>Parent: emit UpdateSelectedFilter(selectedFilter)
Dialog->>Parent: call onDismiss()
Parent->>Dialog: dismiss
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt(0 hunks)feature/search/src/commonMain/kotlin/com/mifos/feature/search/components/FilterDialog.kt(1 hunks)
💤 Files with no reviewable changes (1)
- feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/search/src/commonMain/kotlin/com/mifos/feature/search/components/FilterDialog.kt (1)
feature/search/src/commonMain/kotlin/com/mifos/feature/search/SearchViewModel.kt (1)
onEvent(40-88)
🔇 Additional comments (3)
feature/search/src/commonMain/kotlin/com/mifos/feature/search/components/FilterDialog.kt (3)
37-37: LGTM! Parameter type widened to support new event flow.The change from
(SearchScreenEvent.UpdateSelectedFilter) -> Unitto(SearchScreenEvent) -> Unitcorrectly enables the dialog to dispatch bothUpdateSelectedFilter(for filter selection) andPerformSearch(for explicit search trigger) events, aligning with the PR objective to require explicit user action.
46-49: LGTM! Explicit search trigger correctly implemented.The
onConfirmhandler now properly dispatchesPerformSearchbefore dismissing the dialog. This aligns with the PR objective to require explicit user action and matches the SearchViewModel pattern wherePerformSearchtriggersgetSearchResult().
62-73: LGTM! Filter selection correctly updates state without auto-dismissal.The filter selection now only dispatches
UpdateSelectedFilterevents without automatically dismissing the dialog or triggering a search. This allows users to change their filter selection before explicitly applying it via the "Apply" button, improving the user experience and aligning with the PR objective.
feature/search/src/commonMain/kotlin/com/mifos/feature/search/components/FilterDialog.kt
Outdated
Show resolved
Hide resolved
revanthkumarJ
left a comment
revanthkumarJ
left a comment
There was a problem hiding this comment.
We can merge this one once you move those strings to string resources
| showDialogState = true, | ||
| confirmButtonText = "", | ||
| confirmButtonText = "Apply", | ||
| dismissButtonText = "Close", |
There was a problem hiding this comment.
Move the strings to String Resources.
There was a problem hiding this comment.
I have made some changes can u plz review it again !!
There was a problem hiding this comment.
chamge the All text also to string resources
shashank-0-0
force-pushed
the
fix/search-filter-dialog
branch
from
5885d56 to
5f61f53
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/search/src/commonMain/composeResources/values/feature_search_strings.xml(1 hunks)feature/search/src/commonMain/kotlin/com/mifos/feature/search/components/FilterDialog.kt(4 hunks)
🔇 Additional comments (5)
feature/search/src/commonMain/composeResources/values/feature_search_strings.xml (1)
21-22: LGTM! String resources properly added for internationalization.The addition of
feature_search_applyandfeature_search_closestring resources correctly addresses the previous review feedback about hardcoded button labels and enables proper internationalization support.feature/search/src/commonMain/kotlin/com/mifos/feature/search/components/FilterDialog.kt (4)
13-14: LGTM! Imports properly added for i18n and state management.The string resource imports for
feature_search_applyandfeature_search_closeenable proper internationalization, addressing previous review feedback. The state management imports (getValue,mutableStateOf,remember,setValue) are correctly added to support the local state pattern.Also applies to: 27-30
47-47: LGTM! Local state management improves UX.The introduction of
selectedFilterlocal state is a good UX improvement. It allows users to preview and change their filter selection multiple times before applying, and dismissing the dialog without confirming will discard the changes. This follows the standard confirmation dialog pattern.
52-57: LGTM! Button configuration correctly implements confirmation pattern.The button labels now properly use string resources for internationalization. The
onConfirmcallback correctly emits the filter update event with the local state and then dismisses the dialog, while the dismiss button discards changes. This implements the standard confirmation dialog pattern correctly.
69-71: LGTM! Selection logic correctly updates local state.The filter option selection logic properly updates the local
selectedFilterstate instead of immediately emitting events. The "All" option correctly maps tonull, and the rendering logic accurately reflects the local state. This ensures changes are only applied when the user clicks "Apply".Also applies to: 78-80
feature/search/src/commonMain/kotlin/com/mifos/feature/search/components/FilterDialog.kt
Outdated
Show resolved
Hide resolved
feature/search/src/commonMain/kotlin/com/mifos/feature/search/components/FilterDialog.kt
Outdated
Show resolved
Hide resolved
2 participants
Fixes - Jira-#504
Screen_recording_20251106_222942.webm
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit