Implemented a Export Dialog Box in Transaction Screen#2607
Implemented a Export Dialog Box in Transaction Screen#2607gurnoorpannu wants to merge 3 commits intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughAdds an export UI: six new string resources, a new ExportTransactionsDialog composable with from/to date pickers and validation, and a scaffold action in LoanTransactionsScreen that opens the dialog and returns a selected date range. Changes
Sequence DiagramsequenceDiagram
actor User
participant LoanTransactionsScreen as LTScreen
participant ExportTransactionsDialog as Dialog
participant DatePicker as Picker
User->>LTScreen: Tap export button
LTScreen->>LTScreen: set showExportDialog = true
LTScreen->>Dialog: show dialog
User->>Dialog: Open from-date picker
Dialog->>Picker: request from-date (min 2000-01-01)
Picker->>Dialog: return fromDate (ms)
Dialog->>Dialog: update fromDate
User->>Dialog: Open to-date picker
Dialog->>Picker: request to-date (min 2000-01-01)
Picker->>Dialog: return toDate (ms)
Dialog->>Dialog: update toDate, validate range
alt Valid date range
User->>Dialog: Tap Generate Report
Dialog->>LTScreen: onGenerateReport(fromMs, toMs)
LTScreen->>Dialog: dismiss dialog
else Invalid date range
Dialog->>User: show invalid date range error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
|
I think the the close(x) button should be black with transparent background. |
| private fun formatDateFromMillis(millis: Long?): String { | ||
| if (millis == null) return "" | ||
| val localDate = Instant.fromEpochMilliseconds(millis) | ||
| .toLocalDateTime(TimeZone.currentSystemDefault()) |
There was a problem hiding this comment.
there is a time zone mismatch with fun createSelectableDatesFrom(minDate: LocalDate) = object : SelectableDates I suggest using TimeZone.UTC for consistency
There was a problem hiding this comment.
will fix that, ty for the review :)
|
|
||
| <!-- Export Loan Transactions --> | ||
| <string name="feature_loan_export">Export</string> | ||
| <string name="feature_loan_export_transactions">Export</string> |
There was a problem hiding this comment.
why two different keys with same values ? you meant the second one to be Export Transactions did't you
| } | ||
|
|
||
| @OptIn(ExperimentalTime::class) | ||
| @Composable |
There was a problem hiding this comment.
I think this function should not be annotated with Composable it does not emit UI, call remember or other compose APIs or take compose parameter, marking it with compose it triggers unnecessary recomposition tracking overhead.
| var showToDatePicker by rememberSaveable { mutableStateOf(false) } | ||
| var fromDate: Long? by rememberSaveable { mutableStateOf(null) } | ||
| var toDate: Long? by rememberSaveable { mutableStateOf(null) } | ||
| var showInvalidDateRangeError by remember { mutableStateOf(false) } |
There was a problem hiding this comment.
this will be lost on config change, should not it be rememberSaveable also ?
There was a problem hiding this comment.
@markrizkalla I kept it as remember intentionally since it is just a error message that gets recalculated when the user interacts with the form only the actual date values need to survive config changes
gurnoorpannu
force-pushed
the
feature/loan-transactions-export-dialog
branch
from
377ec90 to
c7cb0e4
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
297-298: Duplicate string values:feature_loan_exportandfeature_loan_export_transactionsare both "Export".These strings serve different UI purposes:
feature_loan_exportis used as a content description for the export button icon (line 136 of LoanTransactionsScreen.kt), whilefeature_loan_export_transactionsis the dialog title (line 134 of ExportTransactionsDialog.kt). Consider differentiating their values—e.g., "Export" for the button and "Export Transactions" for the dialog title—to provide better context in different parts of the UI.
There was a problem hiding this comment.
Hey @gurnoorpannu ! Consider attaching a video (screen recording) for all the possible situations as mentioned in the ticket description-
On tapping Export, show a dialog with:
From Date (required)
To Date (required)
Cancel button
Generate Report button (disabled until validation passes)
Validation rules:
Both dates must be selected
To Date cannot be earlier than From Date
Only after valid input, the Generate Report button should become active
It's hard to validate these things with a single screenshot.
Thanks!
updated it 👍 |
|
@gurnoorpannu Have you validated that if |
4 participants
Fixes - Jira-#657
Before:-
After:-
Recording:-
FILE.2026-02-16.18.20.06.mp4
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