[Woo POS][Design system] Implement design review comments #13615

kidinov · 2025-02-24T14:12:50Z

Closes: #13614

Description

The PR brings multiple changes to address PR review comments

qJpoXnTMMBPsgHBedaBWGv-fi-5_7448

Testing information

Go through Figma comments and validate that they are addressed

The tests that have been performed

Images/gif

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.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

…sList

…ositioning

wpmobilebot · 2025-02-24T14:17:53Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`0c1b5ef`
	Direct Download	`woocommerce-wear-prototype-build-pr13615-0c1b5ef.apk`

wpmobilebot · 2025-02-24T14:34:18Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`0c1b5ef`
	Direct Download	`woocommerce-prototype-build-pr13615-0c1b5ef.apk`

…groundOverlay

kidinov · 2025-02-25T08:40:14Z

@coderabbitai review

…BackgroundOverlay

… instead of free order

kidinov · 2025-02-25T09:37:12Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsScreen.kt

@@ -154,27 +154,36 @@ private fun TotalsLoaded(
        horizontalAlignment = Alignment.CenterHorizontally,
        verticalArrangement = Arrangement.Center,
    ) {
-        if (!state.isFreeOrder) {


@AnirudhBhat

In my opinion, this is not a valid way to build a view model. With this approach, you can have a view model that includes, for instance, reader status "ready for payment" and "isFreeOrder == true," which is an invalid state. I believe we should not model it this way therefore I changed it. wdyt?

Having said that, I don't understand why we even allow an order with price zero collected with cash but not with card? In both cases, it's impossible, correct?

Having said that, I don't understand why we even allow an order with price zero collected with cash but not with card? In both cases, it's impossible, correct?

Stripe wont' allow us to create a payment intent with price 0. I think a proper solution would be to replace the Cash Payment button with something like Complete Order -> change payment success screen, to something like Order Completed screen. However, I'm not sure it's worth the effort considering this flow is not for usual sales, but for rare use-cases in which someone is giving away free stuff but they still want to keep a record of them.

Stripe wont' allow us to create a payment intent with price 0. I think a proper solution would be to replace the Cash Payment button with something like Complete Order -> change payment success screen, to something like Order Completed screen. However, I'm not sure it's worth the effort considering this flow is not for usual sales, but for rare use-cases in which someone is giving away free stuff but they still want to keep a record of them.

I see. Make sense! Thanks

To be honest, I would leave it to the user, as I believe we handle the error when the payment intent was not created. So my understanding is that there will be an error in the top half and a "cash payment" button at the bottom. We added extra complexity just to avoid showing an error, while, in my opinion, the error is perfectly acceptable in this case and maybe even clearer for the user than just missing the card reader section. However, I think that since we already have this code, we can keep it.

So my understanding is that there will be an error in the top half and a "cash payment" button at the bottom.

That's not the case. There are much bigger problem when we enable card payments for free products. You can find the discussion here - p1736742009959369-slack-C070SJRA8DP.

kidinov · 2025-02-25T09:39:21Z

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsScreen.kt

-                    is WooPosTotalsViewState.ReaderStatus.Disconnected -> {
-                        ReaderDisconnected(modifier = Modifier, status = readerStatus, onUIEvent = onUIEvent)
-                    }
+        Column(


The UI for the case when it's 0 priced order is slightly different with Figma, but considering that this is an edge case and that the UI is ok. I think the complexity that it will bring is not worth it

…-review-comments

codecov-commenter · 2025-02-25T10:06:12Z

Codecov Report

Attention: Patch coverage is 38.09524% with 26 lines in your changes missing coverage. Please review.

Project coverage is 38.05%. Comparing base (0a0c70c) to head (0c1b5ef).
Report is 15 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...ce/android/ui/woopos/home/items/WooPosItemsList.kt	0.00%	21 Missing ⚠️
...oid/ui/woopos/home/totals/WooPosTotalsViewState.kt	78.57%	3 Missing ⚠️
...mon/composeui/component/WooPosBackgroundOverlay.kt	0.00%	1 Missing ⚠️
...pos/common/composeui/component/WooPosShimmerBox.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #13615      +/-   ##
============================================
- Coverage     38.06%   38.05%   -0.01%     
+ Complexity     9058     9057       -1     
============================================
  Files          2058     2058              
  Lines        112694   112696       +2     
  Branches      14286    14285       -1     
============================================
- Hits          42898    42891       -7     
- Misses        65895    65904       +9     
  Partials       3901     3901

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

malinajirka

Thanks @kidinov!

The changes all look good to me. The one minor thing I'm not sure about is whether the vertical position of the email/cash value shouldn't be centralized, but we can iterate on that later.

@AnirudhBhat Could you please scan through the changes to the payment states? They look good to me, but better be sure.

P.S. I think it would have been clearer to keep the payment state changes in a separate PR - mostly because, if there is a bug the misleading PR title might throw us off when searching for the PR that introduced the bug. I wouldn't worry about it too much, but I'd personally try to avoid this in the future.

kidinov · 2025-02-26T09:33:36Z

@malinajirka thanks for the review!

if there is a bug the misleading PR title might throw us off when searching for the PR that introduced the bug. I wouldn't worry about it too much

Yep, I agree. I started to modify that because there is a comment from Wagner regarding this state, and I could not resist not adjusting the view model

The one minor thing I'm not sure about is whether the vertical position of the email/cash value shouldn't be centralized, but we can iterate on that later.

That's relatively easy to do, but it will cause "jumping" of them when the keyboard shows/hides. I'll try and see if it looks ok then

AnirudhBhat

I've scanned the payment state code changes. LGTM

malinajirka · 2025-02-26T11:13:42Z

Awesome, thanks @AnirudhBhat!

I've tested the following:

Supported products toolbar
Empty cart state
Loading variations state
Email and Cash receipts screens
Smoke tested IPP
Success Screen

@kidinov I think this PR is good to be merged. I haven't checked two out of the three checkboxes in the Reviewer section. We discussed the (lack of) single-focus in this PR and I don't think it's worth splitting at this point. However, it'd be useful if you added scenarios you tested to the description. Thanks!

kidinov added 5 commits February 24, 2025 11:40

Update banner tint color to use onSurface from primary

d609521

Update WooPosShimmerBox color alpha and refactor layout in WooPosItem…

138fcc5

…sList

Add imePadding to WooPosCashPaymentScreen layout and clean up margins

d23e29a

Add imePadding to WooPosEmailReceiptScreen layout and adjust button p…

eedb9f7

…ositioning

Refactor reader status handling in WooPosTotalsScreen and ViewModel

a99a849

kidinov linked an issue Feb 24, 2025 that may be closed by this pull request

[Woo POS][Design System] Implement design review comments #13614

Closed

kidinov marked this pull request as draft February 24, 2025 14:13

kidinov added 3 commits February 24, 2025 15:49

Fix punctuation in success payment messages in strings.xml

50f06f8

Add color filter to empty cart icon in WooPosCartScreen

8fa487b

Update background overlay color for improved visibility in WooPosBack…

1425b06

…groundOverlay

kidinov added 3 commits February 25, 2025 09:40

Increase background overlay opacity for improved visibility in WooPos…

11720b5

…BackgroundOverlay

Reduce opacity of color filter for empty cart icon in WooPosCartScreen

2a39679

Update assertions in WooPosTotalsViewModelTest to check reader status…

abf141a

… instead of free order

kidinov requested a review from malinajirka February 25, 2025 09:32

kidinov added the feature: point of sale POS project label Feb 25, 2025

kidinov added this to the 21.9 milestone Feb 25, 2025

kidinov commented Feb 25, 2025

View reviewed changes

kidinov marked this pull request as ready for review February 25, 2025 09:40

Merge branch 'trunk' into 13614-woo-posdesign-system-implement-design…

0c1b5ef

…-review-comments

malinajirka self-assigned this Feb 26, 2025

malinajirka approved these changes Feb 26, 2025

View reviewed changes

AnirudhBhat approved these changes Feb 26, 2025

View reviewed changes

malinajirka merged commit a009714 into trunk Feb 26, 2025
18 of 23 checks passed

malinajirka deleted the 13614-woo-posdesign-system-implement-design-review-comments branch February 26, 2025 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Woo POS][Design system] Implement design review comments #13615

[Woo POS][Design system] Implement design review comments #13615

kidinov commented Feb 24, 2025 •

edited by malinajirka

Loading

wpmobilebot commented Feb 24, 2025 •

edited

Loading

wpmobilebot commented Feb 24, 2025 •

edited

Loading

kidinov commented Feb 25, 2025

kidinov Feb 25, 2025 •

edited

Loading

malinajirka Feb 26, 2025

kidinov Feb 26, 2025

AnirudhBhat Feb 26, 2025

kidinov Feb 25, 2025

codecov-commenter commented Feb 25, 2025 •

edited

Loading

malinajirka left a comment

kidinov commented Feb 26, 2025

AnirudhBhat left a comment

malinajirka commented Feb 26, 2025

[Woo POS][Design system] Implement design review comments #13615

[Woo POS][Design system] Implement design review comments #13615

Conversation

kidinov commented Feb 24, 2025 • edited by malinajirka Loading

Description

Testing information

The tests that have been performed

Images/gif

Reviewer (or Author, in the case of optional code reviews):

wpmobilebot commented Feb 24, 2025 • edited Loading

wpmobilebot commented Feb 24, 2025 • edited Loading

kidinov commented Feb 25, 2025

kidinov Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

malinajirka Feb 26, 2025

Choose a reason for hiding this comment

kidinov Feb 26, 2025

Choose a reason for hiding this comment

AnirudhBhat Feb 26, 2025

Choose a reason for hiding this comment

kidinov Feb 25, 2025

Choose a reason for hiding this comment

codecov-commenter commented Feb 25, 2025 • edited Loading

Codecov Report

malinajirka left a comment

Choose a reason for hiding this comment

kidinov commented Feb 26, 2025

AnirudhBhat left a comment

Choose a reason for hiding this comment

malinajirka commented Feb 26, 2025

kidinov commented Feb 24, 2025 •

edited by malinajirka

Loading

wpmobilebot commented Feb 24, 2025 •

edited

Loading

wpmobilebot commented Feb 24, 2025 •

edited

Loading

kidinov Feb 25, 2025 •

edited

Loading

codecov-commenter commented Feb 25, 2025 •

edited

Loading