-
Notifications
You must be signed in to change notification settings - Fork 132
[Woo POS] Products grid #11539
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
[Woo POS] Products grid #11539
Changes from 7 commits
2821174
b0b4952
975003c
8d33e57
437c925
29db876
8f716e6
ae95e06
dd36fbe
3d41aef
f30af43
65894b9
172e67c
f4760a6
efbab27
0773718
76056fd
94ec44a
1a102c1
c5dbaf2
e20e629
9896ea7
4c7d9c8
ae56422
081df9c
d522c92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,48 +1,120 @@ | ||
package com.woocommerce.android.ui.woopos.cart | ||
|
||
import android.annotation.SuppressLint | ||
import androidx.compose.foundation.layout.Box | ||
import androidx.compose.foundation.layout.Column | ||
import androidx.compose.foundation.layout.Row | ||
import androidx.compose.foundation.layout.Spacer | ||
import androidx.compose.foundation.layout.fillMaxSize | ||
import androidx.compose.foundation.layout.fillMaxWidth | ||
import androidx.compose.foundation.layout.padding | ||
import androidx.compose.material.Button | ||
import androidx.compose.material.Icon | ||
import androidx.compose.material.MaterialTheme | ||
import androidx.compose.material.Scaffold | ||
import androidx.compose.material.Text | ||
import androidx.compose.material.TopAppBar | ||
import androidx.compose.material.icons.Icons | ||
import androidx.compose.material.icons.filled.Close | ||
import androidx.compose.runtime.Composable | ||
import androidx.compose.ui.Alignment | ||
import androidx.compose.ui.Modifier | ||
import androidx.compose.ui.text.style.TextAlign | ||
import androidx.compose.ui.unit.dp | ||
import com.woocommerce.android.ui.woopos.cart.products.ProductSelector | ||
import com.woocommerce.android.ui.woopos.cart.products.ProductSelectorViewModel | ||
import com.woocommerce.android.ui.woopos.util.WooPosPreview | ||
import kotlinx.coroutines.flow.MutableStateFlow | ||
import kotlinx.coroutines.flow.StateFlow | ||
|
||
@Composable | ||
@Suppress("UNUSED_PARAMETER") | ||
fun WooPosCartScreen( | ||
viewModel: WooPosCartViewModel, | ||
cartViewModel: WooPosCartViewModel, | ||
productsViewModel: ProductSelectorViewModel, | ||
onCheckoutClick: () -> Unit, | ||
) { | ||
WooPosCartScreen( | ||
onButtonClicked = onCheckoutClick | ||
onButtonClicked = onCheckoutClick, | ||
productsState = productsViewModel.viewState, | ||
onLoadMore = productsViewModel::onLoadMore, | ||
) | ||
} | ||
|
||
@SuppressLint("UnusedMaterialScaffoldPaddingParameter") | ||
@Composable | ||
private fun WooPosCartScreen(onButtonClicked: () -> Unit) { | ||
private fun WooPosCartScreen( | ||
onButtonClicked: () -> Unit, | ||
productsState: StateFlow<ProductSelectorViewModel.ViewState>, | ||
onLoadMore: () -> Unit, | ||
) { | ||
Box( | ||
Modifier.fillMaxSize(), | ||
contentAlignment = Alignment.Center | ||
) { | ||
Column { | ||
Text( | ||
text = "Cart", | ||
style = MaterialTheme.typography.h3, | ||
color = MaterialTheme.colors.primary, | ||
) | ||
Button(onClick = onButtonClicked) { | ||
Text("Checkout") | ||
Scaffold( | ||
topBar = { | ||
TopAppBar( | ||
navigationIcon = { | ||
Icon( | ||
imageVector = Icons.Default.Close, | ||
contentDescription = "Close POS" | ||
) | ||
}, | ||
title = { | ||
Text( | ||
modifier = Modifier.fillMaxWidth(), | ||
text = "⚠️ Reader not connected", | ||
style = MaterialTheme.typography.h5, | ||
color = MaterialTheme.colors.onPrimary, | ||
textAlign = TextAlign.Center, | ||
) | ||
}, | ||
actions = { | ||
Text(text = "History") | ||
} | ||
) | ||
}, | ||
) { | ||
Row( | ||
modifier = Modifier.padding(16.dp) | ||
) { | ||
ProductSelector(productsState, onLoadMore) | ||
Cart(onButtonClicked) | ||
} | ||
} | ||
} | ||
} | ||
|
||
@Composable | ||
private fun Cart(onButtonClicked: () -> Unit) { | ||
Column { | ||
Text( | ||
text = "Cart", | ||
style = MaterialTheme.typography.h3, | ||
color = MaterialTheme.colors.primary, | ||
) | ||
Spacer(modifier = Modifier.weight(1f)) | ||
Button( | ||
modifier = Modifier.fillMaxWidth(), | ||
onClick = onButtonClicked, | ||
) { | ||
Text("Checkout") | ||
} | ||
} | ||
} | ||
|
||
@Composable | ||
@WooPosPreview | ||
fun WooPosCartScreenPreview() { | ||
WooPosCartScreen(onButtonClicked = {}) | ||
val productState = MutableStateFlow( | ||
ProductSelectorViewModel.ViewState( | ||
products = listOf( | ||
ProductSelectorViewModel.ListItem(1, "Product 1"), | ||
ProductSelectorViewModel.ListItem(2, "Product 2"), | ||
ProductSelectorViewModel.ListItem(3, "Product 3"), | ||
) | ||
) | ||
) | ||
WooPosCartScreen(onButtonClicked = {}, productsState = productState, onLoadMore = {}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
package com.woocommerce.android.ui.woopos.cart.products | ||
|
||
import androidx.compose.foundation.background | ||
import androidx.compose.foundation.layout.Arrangement | ||
import androidx.compose.foundation.layout.fillMaxWidth | ||
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.rememberLazyGridState | ||
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.util.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<ProductSelectorViewModel.ViewState>, | ||
onLoadMore: () -> Unit, | ||
) { | ||
ConstraintLayout( | ||
modifier = Modifier.fillMaxWidth(0.7f) | ||
samiuelson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) { | ||
val state = productsState.collectAsState() | ||
val gridState = rememberLazyGridState() | ||
LazyVerticalGrid( | ||
columns = GridCells.Adaptive(minSize = 128.dp), | ||
samiuelson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
horizontalArrangement = Arrangement.spacedBy(4.dp), | ||
verticalArrangement = Arrangement.spacedBy(4.dp), | ||
state = gridState | ||
) { | ||
items( | ||
count = state.value.products.size, | ||
key = { index -> state.value.products[index].productId } | ||
) { index -> | ||
ProductItem(product = state.value.products[index]) | ||
} | ||
} | ||
InfiniteGridHandler(gridState) { | ||
onLoadMore() | ||
} | ||
} | ||
} | ||
|
||
@Composable | ||
fun ProductItem(product: ProductSelectorViewModel.ListItem) { | ||
ConstraintLayout( | ||
modifier = Modifier.background(Color.Yellow) | ||
) { | ||
Text( | ||
text = product.title, | ||
style = MaterialTheme.typography.body1, | ||
color = MaterialTheme.colors.onSurface, | ||
) | ||
} | ||
} | ||
|
||
@Composable | ||
fun InfiniteGridHandler(gridState: LazyGridState, buffer: Int = 1, onLoadMore: () -> 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) { | ||
samiuelson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
snapshotFlow { loadMore.value } | ||
.distinctUntilChanged() | ||
.filter { it } | ||
.collect { | ||
onLoadMore() | ||
} | ||
} | ||
} | ||
|
||
@Composable | ||
@WooPosPreview | ||
fun ProductSelectorPreview() { | ||
val state = MutableStateFlow( | ||
ProductSelectorViewModel.ViewState( | ||
listOf( | ||
ProductSelectorViewModel.ListItem(1, "Product 1"), | ||
ProductSelectorViewModel.ListItem(2, "Product 2"), | ||
ProductSelectorViewModel.ListItem(3, "Product 3"), | ||
) | ||
) | ||
) | ||
ProductSelector(productsState = state, onLoadMore = {}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
package com.woocommerce.android.ui.woopos.cart.products | ||
|
||
import androidx.lifecycle.SavedStateHandle | ||
import com.woocommerce.android.viewmodel.ScopedViewModel | ||
import dagger.hilt.android.lifecycle.HiltViewModel | ||
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) { | ||
|
||
val viewState: StateFlow<ViewState> = productsDataSource.products.map { products -> | ||
samiuelson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ViewState( | ||
products = products.map { product -> | ||
ListItem( | ||
productId = product.remoteId, | ||
title = product.name, | ||
imageUrl = product.firstImageUrl | ||
) | ||
} | ||
) | ||
}.toStateFlow(ViewState(products = emptyList())) | ||
|
||
init { | ||
launch { | ||
productsDataSource.loadProducts() | ||
} | ||
} | ||
|
||
fun onLoadMore() { | ||
launch { | ||
productsDataSource.loadMore() | ||
} | ||
} | ||
|
||
data class ViewState( | ||
samiuelson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
val products: List<ListItem>, | ||
) | ||
|
||
data class ListItem( | ||
val productId: Long, | ||
val title: String, | ||
val imageUrl: String? = null, | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
package com.woocommerce.android.ui.woopos.cart.products | ||
|
||
import com.woocommerce.android.model.Product | ||
import kotlinx.coroutines.flow.Flow | ||
|
||
interface ProductsDataSource { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I frankly don't see why we need an interface in this case. |
||
val products: Flow<List<Product>> | ||
suspend fun loadProducts() | ||
suspend fun loadMore() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package com.woocommerce.android.ui.woopos.cart.products | ||
|
||
import com.woocommerce.android.model.Product | ||
import com.woocommerce.android.ui.products.selector.ProductListHandler | ||
import kotlinx.coroutines.flow.Flow | ||
import javax.inject.Inject | ||
|
||
class ProductsDataSourceImpl @Inject constructor(private val handler: ProductListHandler) : ProductsDataSource { | ||
override val products: Flow<List<Product>> = handler.productsFlow | ||
|
||
override suspend fun loadProducts() { | ||
handler.loadFromCacheAndFetch(searchType = ProductListHandler.SearchType.DEFAULT) | ||
} | ||
|
||
override suspend fun loadMore() { | ||
handler.loadMore() | ||
} | ||
} |
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.cart.products.ProductsDataSource | ||
import com.woocommerce.android.ui.woopos.cart.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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 interface ProductsDataSource {
val products: Flow<List<Product>>
suspend fun loadProducts()
suspend fun loadMore()
} For now, employing the existing
An optimistic assumption is that we won't need to modify
No
Yes, any change to
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 commentThe 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 API there is a bit weird though There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Some other considerations related to interface design and DI:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
): ProductsDataSource { | ||
return ProductsDataSourceImpl(handler) | ||
} | ||
} |
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'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 thecart
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?
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.
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
andproductselector
?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 split the cart and product selector into separate packages (4c7d9c8). They are now parts of the Home screen and have separate VMs.