-
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] Create the "Record transaction details in order note" section #11569
Conversation
📲 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
.../kotlin/com/woocommerce/android/ui/payments/changeduecalculator/ChangeDueCalculatorScreen.kt
Show resolved
Hide resolved
…e-due-calculator-ui-part-4 # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/payments/changeduecalculator/ChangeDueCalculatorFragment.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/payments/changeduecalculator/ChangeDueCalculatorScreen.kt
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #11569 +/- ##
============================================
- Coverage 40.34% 40.34% -0.01%
+ Complexity 5200 5199 -1
============================================
Files 1087 1087
Lines 63247 63250 +3
Branches 8668 8669 +1
============================================
- Hits 25520 25519 -1
- Misses 35432 35436 +4
Partials 2295 2295 ☔ View full report in Codecov by Sentry. |
@ThomazFB this should be ready for review now. I've tried to follow the existing UI and patterns in the app but open to suggestions. |
…e-due-calculator-ui-part-4
…r-ui-part-4 [Payment Method Improvements] Create the "Mark Order as Complete" Button
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.
Looks good and works as described! Also, nice work with the unit test coverage! Just left some suggestions over the ViewModel usage, but nothing critical.
Adresses: #11525
Also contains changes from this PR which have already been reviewed:
#11570
Follow up items from that PR are in this ticket at TODOs:
#11583
Description
Add the toggle for "Record transaction details in order note" in the Change Due Calculator screen.
Also addresses some feedback from the previous PR that @JorgeMucientes provided me with:
Testing instructions
Expected: You should see the Change Due screen
No added functionality, that will be added here. UI should look similar to iOS:
These issues should be fixed:
Only one toolbar visible: [Payment Method Improvements] The Payment Details section should display two text fields #11555 (comment)
Fix text field design: [Payment Method Improvements] The Payment Details section should display two text fields #11555 (comment)
Navigation pattern should be in line with the rest of the app: [Payment Method Improvements] The Payment Details section should display two text fields #11555 (comment)
Change due field should not appear to be editable
I have considered if this change warrants release notes and have added them to
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.