[Woo POS] Products grid #11539
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package com.woocommerce.android.ui.woopos.di | ||
|
|
||
| import com.woocommerce.android.ui.products.selector.ProductListHandler | ||
| import com.woocommerce.android.ui.woopos.home.products.ProductsDataSource | ||
| import com.woocommerce.android.ui.woopos.home.products.ProductsDataSourceImpl | ||
| import dagger.Module | ||
| import dagger.Provides | ||
| import dagger.hilt.InstallIn | ||
| import dagger.hilt.components.SingletonComponent | ||
|
|
||
| @InstallIn(SingletonComponent::class) | ||
| @Module | ||
| class POSModule { | ||
| @Provides | ||
| fun provideProductList( | ||
| handler: ProductListHandler | ||
| ): ProductsDataSource { | ||
| return ProductsDataSourceImpl(handler) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| package com.woocommerce.android.ui.woopos.home | ||
|
|
||
| import androidx.hilt.navigation.compose.hiltViewModel | ||
| import androidx.navigation.NavGraphBuilder | ||
| import androidx.navigation.compose.composable | ||
| import com.woocommerce.android.ui.woopos.home.cart.WooPosCartViewModel | ||
| import com.woocommerce.android.ui.woopos.home.products.ProductSelectorViewModel | ||
|
|
||
| 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. |
||
| composable(HOME_ROUTE) { | ||
| val cartViewModel: WooPosCartViewModel = hiltViewModel() | ||
| val productsViewModel: ProductSelectorViewModel = hiltViewModel() | ||
|
|
||
| WooPosHomeScreen( | ||
| cartViewModel = cartViewModel, | ||
| productsViewModel = productsViewModel, | ||
| onCheckoutClick = onCheckoutClick | ||
| ) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| package com.woocommerce.android.ui.woopos.home | ||
|
|
||
| import android.annotation.SuppressLint | ||
| import androidx.compose.foundation.background | ||
| import androidx.compose.foundation.layout.Arrangement | ||
| import androidx.compose.foundation.layout.Row | ||
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.material.MaterialTheme | ||
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.unit.dp | ||
| import com.woocommerce.android.ui.woopos.common.composeui.WooPosPreview | ||
| import com.woocommerce.android.ui.woopos.home.cart.Cart | ||
| import com.woocommerce.android.ui.woopos.home.cart.WooPosCartViewModel | ||
| import com.woocommerce.android.ui.woopos.home.products.ListItem | ||
| import com.woocommerce.android.ui.woopos.home.products.ProductSelector | ||
| import com.woocommerce.android.ui.woopos.home.products.ProductSelectorViewModel | ||
| import com.woocommerce.android.ui.woopos.home.products.ViewState | ||
| import kotlinx.coroutines.flow.MutableStateFlow | ||
| import kotlinx.coroutines.flow.StateFlow | ||
|
|
||
| @Composable | ||
| @Suppress("UNUSED_PARAMETER") | ||
| fun WooPosHomeScreen( | ||
| cartViewModel: WooPosCartViewModel, | ||
| productsViewModel: ProductSelectorViewModel, | ||
| onCheckoutClick: () -> Unit, | ||
| ) { | ||
| WooPosHomeScreen( | ||
| onCheckoutClick = onCheckoutClick, | ||
| productsState = productsViewModel.viewState, | ||
| onEndOfProductsGridReached = productsViewModel::onEndOfProductsGridReached, | ||
| ) | ||
| } | ||
|
|
||
| @SuppressLint("UnusedMaterialScaffoldPaddingParameter") | ||
| @Composable | ||
| private fun WooPosHomeScreen( | ||
| onCheckoutClick: () -> Unit, | ||
| productsState: StateFlow<ViewState>, | ||
| onEndOfProductsGridReached: () -> Unit, | ||
| ) { | ||
| Row( | ||
| modifier = Modifier | ||
| .background(MaterialTheme.colors.background) | ||
| .padding(start = 16.dp, end = 16.dp, top = 16.dp, bottom = 0.dp), | ||
| horizontalArrangement = Arrangement.spacedBy(16.dp), | ||
| ) { | ||
| ProductSelector(productsState, onEndOfProductsGridReached) | ||
| Cart(onCheckoutClick) | ||
| } | ||
| } | ||
|
|
||
| @Composable | ||
| @WooPosPreview | ||
| fun WooPosHomeScreenPreview() { | ||
| val productState = MutableStateFlow( | ||
| ViewState( | ||
| products = listOf( | ||
| ListItem(1, "Product 1"), | ||
| ListItem(2, "Product 2"), | ||
| ListItem(3, "Product 3"), | ||
| ) | ||
| ) | ||
| ) | ||
| WooPosHomeScreen( | ||
| onCheckoutClick = {}, | ||
| productsState = productState, | ||
| onEndOfProductsGridReached = {} | ||
| ) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package com.woocommerce.android.ui.woopos.home.cart | ||
|
|
||
| import androidx.compose.foundation.background | ||
| import androidx.compose.foundation.layout.Column | ||
| import androidx.compose.foundation.layout.Spacer | ||
| import androidx.compose.foundation.layout.fillMaxWidth | ||
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.foundation.shape.RoundedCornerShape | ||
| import androidx.compose.material.Button | ||
| import androidx.compose.material.MaterialTheme | ||
| import androidx.compose.material.Text | ||
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.res.stringResource | ||
| import androidx.compose.ui.unit.dp | ||
| import com.woocommerce.android.R | ||
|
|
||
| @Composable | ||
| fun Cart(onButtonClicked: () -> Unit) { | ||
| Column( | ||
| modifier = Modifier | ||
| .background(MaterialTheme.colors.surface, RoundedCornerShape(8.dp)) | ||
| .padding(16.dp) | ||
| ) { | ||
| Text( | ||
| text = "Cart", | ||
| style = MaterialTheme.typography.h3, | ||
| color = MaterialTheme.colors.primary, | ||
| ) | ||
| Spacer(modifier = Modifier.weight(1f)) | ||
| Button( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| onClick = onButtonClicked, | ||
| ) { | ||
| Text(stringResource(id = R.string.woopos_checkout)) | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| package com.woocommerce.android.ui.woopos.home.products | ||
|
|
||
| import androidx.compose.foundation.background | ||
| import androidx.compose.foundation.border | ||
| import androidx.compose.foundation.layout.Arrangement.spacedBy | ||
| import androidx.compose.foundation.layout.PaddingValues | ||
| import androidx.compose.foundation.layout.fillMaxHeight | ||
| import androidx.compose.foundation.layout.fillMaxWidth | ||
| import androidx.compose.foundation.layout.padding | ||
| import androidx.compose.foundation.lazy.grid.GridCells | ||
| import androidx.compose.foundation.lazy.grid.LazyGridState | ||
| import androidx.compose.foundation.lazy.grid.LazyVerticalGrid | ||
| import androidx.compose.foundation.lazy.grid.itemsIndexed | ||
| import androidx.compose.foundation.lazy.grid.rememberLazyGridState | ||
| import androidx.compose.foundation.shape.RoundedCornerShape | ||
| import androidx.compose.material.MaterialTheme | ||
| import androidx.compose.material.Text | ||
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.runtime.LaunchedEffect | ||
| import androidx.compose.runtime.collectAsState | ||
| import androidx.compose.runtime.derivedStateOf | ||
| import androidx.compose.runtime.remember | ||
| import androidx.compose.runtime.snapshotFlow | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.graphics.Color | ||
| import androidx.compose.ui.unit.dp | ||
| import androidx.constraintlayout.compose.ConstraintLayout | ||
| import com.woocommerce.android.ui.woopos.common.composeui.WooPosPreview | ||
| import kotlinx.coroutines.flow.MutableStateFlow | ||
| import kotlinx.coroutines.flow.StateFlow | ||
| import kotlinx.coroutines.flow.distinctUntilChanged | ||
| import kotlinx.coroutines.flow.filter | ||
|
|
||
| @Composable | ||
| fun ProductSelector( | ||
| productsState: StateFlow<ViewState>, | ||
| onEndOfProductsGridReached: () -> Unit, | ||
| ) { | ||
| ConstraintLayout( | ||
| modifier = Modifier | ||
| .fillMaxWidth(0.7f) | ||
| .fillMaxHeight() | ||
| .background(MaterialTheme.colors.surface, RoundedCornerShape(8.dp)) | ||
| .padding(16.dp) | ||
| ) { | ||
| val state = productsState.collectAsState() | ||
| val gridState = rememberLazyGridState() | ||
| LazyVerticalGrid( | ||
| columns = GridCells.Fixed(2), | ||
| horizontalArrangement = spacedBy(34.dp), | ||
| verticalArrangement = spacedBy(34.dp), | ||
| contentPadding = PaddingValues(16.dp), | ||
| state = gridState | ||
| ) { | ||
| itemsIndexed(state.value.products) { _, product -> | ||
| ProductItem(product = product) | ||
| } | ||
| } | ||
| InfiniteGridHandler(gridState) { | ||
| onEndOfProductsGridReached() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Composable | ||
| fun ProductItem(product: ListItem) { | ||
| ConstraintLayout( | ||
| modifier = Modifier | ||
| .border(1.dp, Color.Gray, shape = RoundedCornerShape(4.dp)) | ||
| .padding(16.dp) | ||
| .fillMaxWidth(0.5f) | ||
| ) { | ||
| Text( | ||
| text = product.title, | ||
| style = MaterialTheme.typography.body1, | ||
| color = MaterialTheme.colors.onSurface, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| @Composable | ||
| fun InfiniteGridHandler( | ||
| gridState: LazyGridState, | ||
| buffer: Int = 1, | ||
| onEndOfProductsGridReached: () -> Unit | ||
| ) { | ||
| val loadMore = remember { | ||
| derivedStateOf { | ||
| val layoutInfo = gridState.layoutInfo | ||
| val totalItemsNumber = layoutInfo.totalItemsCount | ||
| val lastVisibleItemIndex = (layoutInfo.visibleItemsInfo.lastOrNull()?.index ?: 0) + 1 | ||
|
|
||
| lastVisibleItemIndex > (totalItemsNumber - buffer) | ||
| } | ||
| } | ||
|
|
||
| LaunchedEffect(loadMore) { | ||
| snapshotFlow { loadMore.value } | ||
| .distinctUntilChanged() | ||
| .filter { it } | ||
| .collect { | ||
| onEndOfProductsGridReached() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Composable | ||
| @WooPosPreview | ||
| fun ProductSelectorPreview() { | ||
| val state = MutableStateFlow( | ||
| ViewState( | ||
| listOf( | ||
| ListItem(1, "Product 1"), | ||
| ListItem(2, "Product 2"), | ||
| ListItem(3, "Product 3"), | ||
| ) | ||
| ) | ||
| ) | ||
| ProductSelector(productsState = state, onEndOfProductsGridReached = {}) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| package com.woocommerce.android.ui.woopos.home.products | ||
|
|
||
| import androidx.lifecycle.SavedStateHandle | ||
| import com.woocommerce.android.viewmodel.ScopedViewModel | ||
| import dagger.hilt.android.lifecycle.HiltViewModel | ||
| import kotlinx.coroutines.Job | ||
| import kotlinx.coroutines.flow.StateFlow | ||
| import kotlinx.coroutines.flow.map | ||
| import kotlinx.coroutines.launch | ||
| import javax.inject.Inject | ||
|
|
||
| @HiltViewModel | ||
| class ProductSelectorViewModel @Inject constructor( | ||
| private val productsDataSource: ProductsDataSource, | ||
| savedStateHandle: SavedStateHandle, | ||
| ) : ScopedViewModel(savedStateHandle) { | ||
|
|
||
| private var loadMoreProductsJob: Job? = null | ||
|
|
||
| val viewState: StateFlow<ViewState> = productsDataSource.products.map { products -> | ||
| ViewState( | ||
| products = products.map { product -> | ||
| ListItem( | ||
| productId = product.remoteId, | ||
| title = product.name, | ||
| imageUrl = product.firstImageUrl | ||
| ) | ||
| } | ||
| ) | ||
| }.toStateFlow(ViewState(products = emptyList())) | ||
|
|
||
| init { | ||
| launch { | ||
| productsDataSource.loadProducts() | ||
| } | ||
| } | ||
|
|
||
| fun onEndOfProductsGridReached() { | ||
| loadMoreProductsJob?.cancel() | ||
| loadMoreProductsJob = launch { | ||
| productsDataSource.loadMore() | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
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 raising these questions, @malinajirka. Let me share my POV.
My reasoning is we won't need to revert this decision. We may want to write a new implementation of the
ProductsDataSourceinterface (I don't see a clear reason for doing it now though):For now, employing the existing
ProductListHandler's code to implement the interface saves us time. I think the important question could be – is theProductsDataSourcedesigned well for our use case?An optimistic assumption is that we won't need to modify
ProductListHandlerimplementation 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.No
Yes, any change to
ProductListHandlerwill 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.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me this is the main question. I personally haven't worked much with
ProductListHandler,ProductSelectorRepositoryandWCProductStore. 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 though
loadFromCacheAndFetchandloadMorereturnsResult<Unit>which we don't use at the momentUh oh!
There was an error while loading. Please reload this page.
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.
Good point about unused returns – we don't handle errors in this PR. I think we should add some sort of
Resultreturn types toloadProducts()andloadMore()functions and handle errors in the VM (I added a task for it).Some other considerations related to interface design and DI:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is more design question, but I think yes we should
I'd invalidate that on swipe to refresh gesture after success comes from the backend
Won't see keep what we show in memory in VM anyway?
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.
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.