[Woo POS] Products grid #11539
[Woo POS] Products grid #11539
Conversation
Generated by 🚫 Danger |
samiuelson
changed the title
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
| class POSModule { | ||
| @Provides | ||
| fun provideProductList( | ||
| handler: ProductListHandler |
There was a problem hiding this comment.
Seems like we should indeed use this as it will be faster than making a new class and so far there's no differences with POS requirements. I think we can swap it out later if the POS functionality requires changes.
There was a problem hiding this comment.
Some questions that come to mind which might be worth answering: How complex will it be to revert this decision later? What are the risks of affecting the store-management section of the app (eg. introducing new bugs, etc)? Does this decision make the store-management section more complex? Does it increase maintainability (eg. more testing is needed, code is more difficult to reason about, ...)? What are reasons for change to the ProductListHandler - is it dependant on the use-case (POS vs store-management) or is it a generic component?
Just to be clear, I'm not challenging whether we should re-use or not, since I honestly don't know for this case. I'm just trying to better understand the implications. In general, I'm all up for re-using where we can, but at the same time I don't think we should re-use just because the code can be invoked from two different sections of the app and works (now) -> the reason for change should imo be the same for all the sections in which the code is used or the code probably shouldn't be re-used.
There was a problem hiding this comment.
Thanks for raising these questions, @malinajirka. Let me share my POV.
How complex will it be to revert this decision later?
My reasoning is we won't need to revert this decision. We may want to write a new implementation of the ProductsDataSource interface (I don't see a clear reason for doing it now though):
interface ProductsDataSource {
val products: Flow<List<Product>>
suspend fun loadProducts()
suspend fun loadMore()
}For now, employing the existing ProductListHandler's code to implement the interface saves us time. I think the important question could be – is the ProductsDataSource designed well for our use case?
What are the risks of affecting the store-management section of the app (eg. introducing new bugs, etc)?
An optimistic assumption is that we won't need to modify ProductListHandler implementation at all, which saves us development effort now. If we have to modify it in the future though, I'd consider forking/copying this class and adjusting it for the POS-specific purpose, or implementing from scratch. That said the store management app will not be affected at all.
Does this decision make the store-management section more complex?
No
Does it increase maintainability (eg. more testing is needed, code is more difficult to reason about, ...)?
Yes, any change to ProductListHandler will be reflected in both POS and store management modes. It will require additional awareness. If we want to avoid this, we can consider forking/copying this class.
What are reasons for change to the ProductListHandler - is it dependant on the use-case (POS vs store-management) or is it a generic component?
My POV is that ProductListHandler is a generic component, mature, and with a narrow scope of responsibilities – a helper class for handling product pagination and search modes.
Let me know what you think, @malinajirka, @kidinov, @backwardstruck.
There was a problem hiding this comment.
I think the important question could be – is the ProductsDataSource designed well for our use case?
To me this is the main question. I personally haven't worked much with ProductListHandler, ProductSelectorRepository and WCProductStore. Is this any good? Idon't recall any complains that products are missing or not loading or something like that, so maybe a good sign. And also there is a good sign it doesn't do 10 network requests when products screen is open in the current app =)
API there is a bit weird thoughloadFromCacheAndFetch and loadMore returns Result<Unit> which we don't use at the moment
There was a problem hiding this comment.
Good point about unused returns – we don't handle errors in this PR. I think we should add some sort of Result return types to loadProducts() and loadMore() functions and handle errors in the VM (I added a task for it).
Some other considerations related to interface design and DI:
- Do we want to support swipe to refresh
- Do we need a possibility to invalidate the data
- Do we want
ProductsDataSourceto be a singleton? This works as a naive in-memory data cache (optimizes perfomance), but also prevents from reloading products (risk of data being stale; also a problem in the app currently)
There was a problem hiding this comment.
Do we want to support swipe to refresh
I guess this is more design question, but I think yes we should
Do we need a possibility to invalidate the data
I'd invalidate that on swipe to refresh gesture after success comes from the backend
Do we want ProductsDataSource to be a singleton? This works as a naive in-memory data cache (optimizes perfomance), but also prevents from reloading products (risk of data being stale; also a problem in the app currently)
Won't see keep what we show in memory in VM anyway?
There was a problem hiding this comment.
An optimistic assumption is that we won't need to modify ProductListHandler implementation at all, which saves us development effort now. If we have to modify it in the future though, I'd consider forking/copying this class and adjusting it for the POS-specific purpose, or implementing from scratch. That said the store management app will not be affected at all.
I think one more factor which might play a role here is that we might not be the developers who will need to modify the code in the future. Will the other devs realize, they should create a copy/replace/refactor the component instead of branching POS vs not-POS and therefore coupling these two modes?
Having said all that, I'm personally also inclined towards re-using it, since as you said My POV is that ProductListHandler is a generic component, mature, and with a narrow scope of responsibilities – a helper class for handling product pagination and search modes.. But in general, I think our approach to re-using needs to consider future implications and whether we are setting future-us and other-devs for a success or a failure.
…grid-cart-top-app-bar
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #11539 +/- ##
============================================
- Coverage 40.44% 40.39% -0.05%
Complexity 5196 5196
============================================
Files 1083 1087 +4
Lines 62996 63068 +72
Branches 8628 8642 +14
============================================
Hits 25479 25479
- Misses 35224 35296 +72
Partials 2293 2293 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @samiuelson! I've done a quick pass and left some questions. Please bear in mind that I'm not that familiar with Compose and not at all familiar with Compose navigation, so my comments might not make sense.
Overall, I didn't spot anything which would be blocking the merge. I'm approving it and leaving the merge to you or others.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/cart/products/ProductSelector.kt
Outdated
Show resolved
Hide resolved
| @Composable | ||
| private fun WooPosCartScreen(onButtonClicked: () -> Unit) { | ||
| private fun WooPosCartScreen( |
There was a problem hiding this comment.
🤔 I'm wondering if we could build the cart section and the product selector more independently. It seems we are assuming they'll always be on the same screen. Or perhaps the part which makes this a bit confusing for me is that onLoadMore feels irrelevant to the cart section but still is one of the parameters to WooPosCartScreen.
P.S. I realize this is just a skeleton, but 'onButtonClicked' feels like a very generic name. Could we perhaps rename it to something more specific to make the code easier to reason about?
There was a problem hiding this comment.
I'd also reconsider packaging here and naming. Maybe call the screen with scaffold CartBuildScreen (and package as well) and inside of it can have 2 packages: cart and productselector?
There was a problem hiding this comment.
I split the cart and product selector into separate packages (4c7d9c8). They are now parts of the Home screen and have separate VMs.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/cart/products/ProductSelector.kt
Outdated
Show resolved
Hide resolved
| class POSModule { | ||
| @Provides | ||
| fun provideProductList( | ||
| handler: ProductListHandler |
There was a problem hiding this comment.
Some questions that come to mind which might be worth answering: How complex will it be to revert this decision later? What are the risks of affecting the store-management section of the app (eg. introducing new bugs, etc)? Does this decision make the store-management section more complex? Does it increase maintainability (eg. more testing is needed, code is more difficult to reason about, ...)? What are reasons for change to the ProductListHandler - is it dependant on the use-case (POS vs store-management) or is it a generic component?
Just to be clear, I'm not challenging whether we should re-use or not, since I honestly don't know for this case. I'm just trying to better understand the implications. In general, I'm all up for re-using where we can, but at the same time I don't think we should re-use just because the code can be invoked from two different sections of the app and works (now) -> the reason for change should imo be the same for all the sections in which the code is used or the code probably shouldn't be re-used.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/cart/products/ProductSelector.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/com/woocommerce/android/ui/woopos/cart/products/ProductSelectorViewModel.kt
Outdated
Show resolved
Hide resolved
| import com.woocommerce.android.model.Product | ||
| import kotlinx.coroutines.flow.Flow | ||
|
|
||
| interface ProductsDataSource { |
There was a problem hiding this comment.
I frankly don't see why we need an interface in this case. ProductsDataSourceImpl defines an interface with it's public methods anyway
.../src/main/kotlin/com/woocommerce/android/ui/woopos/cart/products/ProductSelectorViewModel.kt
Outdated
Show resolved
Hide resolved
|
I'm converting it to draft PR because we want to focus first on implementing the whole navigation flow rather than individual screens. This PR can be reused or serve as inspiration for implementing the product selector pane though. |
@samiuelson Could you please share an additional context about this direction - a link to a related conversation is fine. Thanks! |
@malinajirka, p1717001202981269/1716809008.635649-slack-C070SJRA8DP During the POS sync meeting we analyzed how the UX concepts are evolving (i1 vs i2) and realized that until the UX flow is final we won't know exactly how to share state between screens/panes. That's why we asked to speed up the final decision on the UX last week. I'm resuming the work on this PR as we have clarity on the navigation. |
samiuelson
changed the base branch from
trunk
to
11644-woo-pos-implement-skeleton-of-the-i3-of-the-wireframes
samiuelson
changed the base branch from
trunk
to
11644-woo-pos-implement-skeleton-of-the-i3-of-the-wireframes-2
…grid-cart-top-app-bar
|
👋🏼 @malinajirka, @kidinov I've addressed your comments, and updated the branch with the Screen_recording_20240604_104350.mp4In the next iteration, I will focus on syncing item selection state between Cart and Product Selector and creating the Order. |
|
|
||
| internal const val HOME_ROUTE = "home" | ||
|
|
||
| internal fun NavGraphBuilder.homeScreen(onCheckoutClick: () -> Unit) { |
There was a problem hiding this comment.
I changed "Cart" to "Home" as the cart is only one of the panes on the home screen.
There was a problem hiding this comment.
@samiuelson Thank you for working on this. I've reviewed the PR and re-tested it locally - I think it's a great prototype we can iterate on. I'd merge it, but it seems the CI is failing - feel free to merge it after that's fixed.
samiuelson
changed the base branch from
11644-woo-pos-implement-skeleton-of-the-i3-of-the-wireframes-2
to
trunk
samiuelson
deleted the
11516-woo-pos-core-home-screen-ui--product-grid-cart-top-app-bar
branch
Closes: #11516
Description
This PR adds a very basic grid view showing products at the left side of the Cart screen. The product grid supports pagination.
Not included:
The above will come in the next PRs for the sake of brevity.
Testing instructions
Test if all the products are loading. At the current stage all products should be visible, not only simple products.
Images/gif
grid.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.