-
Notifications
You must be signed in to change notification settings - Fork 133
[POS Orders] Empty and Error states #14695
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
base: trunk
Are you sure you want to change the base?
Conversation
text = it.text, | ||
onClick = it.click, | ||
modifier = Modifier | ||
.fillMaxWidth(.5f) |
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 In this case (and in the empty state), the button has a narrower width in the design. Let me know if you have a better idea for how to adapt it.
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.
That changes the state on all 11 places where this component is used. If this is what Wagner's idea was, then it's fine. Overall, I think it looks ok
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.
We might want to extract that constant so that changing the width will only require one change. This is because both buttons must have the same width
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #14695 +/- ##
=========================================
Coverage 38.37% 38.38%
- Complexity 9835 9836 +1
=========================================
Files 2093 2093
Lines 116753 116757 +4
Branches 15630 15630
=========================================
+ Hits 44809 44812 +3
- Misses 67781 67782 +1
Partials 4163 4163 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -0,0 +1,25 @@ | |||
<vector xmlns:android="http://schemas.android.com/apk/res/android" |
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.
val onBackClicked = { onNavigationEvent(WooPosNavigationEvent.GoBack) } | ||
BackHandler { onNavigationEvent(WooPosNavigationEvent.GoBack) } | ||
|
||
if (state is WooPosOrdersState.Error) { |
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.
Generally, sealed class -> when statement without else -> compilation time check that all cases are handled.
And I think there’s some issue with structure. Your top-level state goes down to what is "Content" state, while you still treat as top-level WooPosOrdersState
state
The toolbar should be visible all the time, not rerendered on the state change. Also there are some naming issues. I tried to address those in the patch, please let me know wdyt?
Refactor_WooPosOrdersScreen_to_improve_state_handling_and_simplify_composables.patch
} | ||
|
||
fun onOrdersEmptyActionClicked() { | ||
// Action to be defined |
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.
Just sharing my perspective - I think the only CTA needed on an empty state is "create a new something" one, and here it doesn’t apply. I think only what we need is to have pull to refresh enabled on this state, and that's it. There is nothing to "learn more" here.
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.
Thanks for your work here! I left a few comments; please take a look and let me know what do you think?
Part of WOOMOB-1149
Description
With this PR we handle the error and empty cases when loading POS Orders. The CTA for the empty state is still to be defined.
Steps to reproduce
Empty State
With a store without POS orders
Error State
Images/gif
Screen_Recording_20251003_155502_Woo.Dev.mp4
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.