-
Notifications
You must be signed in to change notification settings - Fork 135
Order details show refunded shipping lines in refund summary #6721
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
Order details show refunded shipping lines in refund summary #6721
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
Generated by 🚫 dangerJS |
I think Android's font is more forgiving than iOS's! Removing the word "and" helps too, even though it doesn't seem quite as polished. On iOS, the English text still reaches more than half way across the row with a comma instead of "and", so I'm still worried about truncation for localizations with longer words or in larger dynamic type sizes. Joe is taking a look at other options too. Another difference: on iOS we style these lines with colour, to indicate that they can be tapped to show more detail. Has Android never had that? Perhaps they should have a disclosure indicator instead, but perhaps the reason they don't is that it would take the amounts out of alignment in the tableview. |
|
You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code: |
🤯
I think an indicator of some sort would be a lot more accessible, but you’re right about it being tricky to find a spot for it. I’ll add it to my list of things to think about. |
yep, on android it's not clickable at all. Just the text |
| } | ||
|
|
||
| @Test | ||
| fun `given refund with product, shipping and fee, when building line, then product shipping and fee returned`() { |
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.
❓ What do you think about writing tests that verify multiple products?
Like
"2 products, shipping"
"5 products, fees"
"10 products, shipping, fees"
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.
Added more tests here - 7677549
AnirudhBhat
left a comment
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.
@kidinov Thank you for working on this! I've tested the changes and it works as expected. The code changes look good as well. I've left a minor comment related to unit tests and it is not blocking at all.
I read in the linked issue that the designs are not finalized for this. I'll not merge this PR as of now. Feel free to merge this if you think that the design part can be taken in a separate PR.
|
@joe-keenan @joshheald I'll merge this PR for now (it goes to a feature branch so no changes to production) and add a comment to our P2 with the changes between iOS and Android here so we can decide on what platform extra changes are needed |

Closes: #6714
Description
Adds more info in the refund lines.
Possible cases:
@joe-keenan @joshheald wdyt about this approach?
Testing instructions
Images/gif