-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Payment Method Improvements] The Payment Details section should display two text fields #11555
Conversation
…nts' into 11525-change-due-calculator-ui-part-2 # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/payments/methodselection/ChangeDueCalculatorFragment.kt
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
.../kotlin/com/woocommerce/android/ui/payments/changeduecalculator/ChangeDueCalculatorScreen.kt
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #11555 +/- ##
=========================================
Coverage 40.33% 40.33%
- Complexity 5200 5201 +1
=========================================
Files 1087 1087
Lines 63224 63224
Branches 8667 8666 -1
=========================================
+ Hits 25499 25501 +2
+ Misses 35430 35429 -1
+ Partials 2295 2294 -1 ☔ View full report in Codecov by Sentry. |
Recommend preferring Android platform convention over exactly matching the iOS implementation. |
.../kotlin/com/woocommerce/android/ui/payments/changeduecalculator/ChangeDueCalculatorScreen.kt
Show resolved
Hide resolved
.../kotlin/com/woocommerce/android/ui/payments/changeduecalculator/ChangeDueCalculatorScreen.kt
Show resolved
Hide resolved
uiState: ChangeDueCalculatorViewModel.UiState, | ||
onNavigateUp: () -> Unit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing against this approach of passing the state and any navigation callback to the Composable function. Especially because, in this case, the screen seems to be simple in terms of navigation. But in case you want to check the pattern we usually use for navigation when using composable UI you might want to check screens like StoreOnboardingFragment
or GetPaidFragment
. I'd say most of the screens where we use compose follow the pattern:
- Passing the viewModel to the root composable function
- Handle navigation events in the host fragment by observing events triggered from the ViewModel.
Another option is to handle navigation directly from composable code, which is something we started doing recently like in DashboardReviewsCard.
Feel free to adopt whatever you think is best for this case. Just sharing a couple of alternatives we are currently using in the codebase 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion. I will do that. Just need to pick one to implement. Thanks for these options.
Nice job @backwardstruck 🏅 |
This is super helpful. Indeed I missed some things. I will address all the points in the PR which is already targeting this one: #11569 |
Closes:
Closes: #11553
Description
This pull request introduces improvements to the Change Due Calculator component. The primary focus of these changes is to enhance user experience.
FYI @soundsokay I think the UI could use more work but I'm not sure how much to copy iOS versus other screens in the Android app. Open to suggestions!
The changes include:
orderId
orderId
to verify that the system appropriately blocks or redirects the user.This update intends to align the UI components with their iOS counterparts.
Testing Instructions
$0.00
Images/GIF
RELEASE-NOTES.txt
if necessary.