Skip to content

[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

Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
2821174
Update WooPosCartScreen
samiuelson May 15, 2024
b0b4952
Merge branch 'trunk' into 11516-woo-pos-core-home-screen-ui--product-…
samiuelson May 16, 2024
975003c
Implement products data source
samiuelson May 17, 2024
8d33e57
Implement base product selector vm
samiuelson May 17, 2024
437c925
Display grid with product titles
samiuelson May 17, 2024
29db876
Address detekt's complaints
samiuelson May 17, 2024
8f716e6
Implement infinite grid loading
samiuelson May 17, 2024
ae95e06
Merge branch 'trunk' into 11516-woo-pos-core-home-screen-ui--product-…
samiuelson May 17, 2024
dd36fbe
Rename function: onLoadMode 👉 onEndOfProductsGridReached
samiuelson May 20, 2024
3d41aef
Rename param
samiuelson May 22, 2024
f30af43
Reorganise packages
samiuelson May 22, 2024
65894b9
Optimise imports
samiuelson May 22, 2024
172e67c
Extract view state class to separate file
samiuelson May 22, 2024
f4760a6
Extract view state class to separate file
samiuelson May 22, 2024
efbab27
Merge remote-tracking branch 'origin/11516-woo-pos-core-home-screen-u…
samiuelson May 22, 2024
0773718
Fix detekt's complaints
samiuelson May 22, 2024
76056fd
Clean up code
samiuelson May 22, 2024
94ec44a
Simplify UI layouts
samiuelson May 22, 2024
1a102c1
Merge branch `11516-woo-pos-core-home-screen-ui--product-grid-cart-to…
samiuelson Jun 3, 2024
c5dbaf2
Update grid UI
samiuelson Jun 3, 2024
e20e629
Update grid UI
samiuelson Jun 3, 2024
9896ea7
Adjust colors for dark/light theme
samiuelson Jun 3, 2024
4c7d9c8
Decouple product selector from cart
samiuelson Jun 4, 2024
ae56422
Merge branch 'trunk' into 11516-woo-pos-core-home-screen-ui--product-…
samiuelson Jun 4, 2024
081df9c
Fix lint
samiuelson Jun 4, 2024
d522c92
Fill product selector's height
samiuelson Jun 4, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,20 @@ package com.woocommerce.android.ui.woopos.cart
import androidx.hilt.navigation.compose.hiltViewModel
import androidx.navigation.NavGraphBuilder
import androidx.navigation.compose.composable
import com.woocommerce.android.ui.woopos.cart.products.ProductSelectorViewModel

internal const val CART_ROUTE = "cart"

internal fun NavGraphBuilder.cartScreen(
onCheckoutClick: () -> Unit
) {
composable(CART_ROUTE) {
val viewModel: WooPosCartViewModel = hiltViewModel()
val cartViewModel: WooPosCartViewModel = hiltViewModel()
val productsViewModel: ProductSelectorViewModel = hiltViewModel()

WooPosCartScreen(
viewModel = viewModel,
cartViewModel = cartViewModel,
productsViewModel = productsViewModel,
onCheckoutClick = onCheckoutClick
)
}
Expand Down
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(
Copy link
Contributor

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 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?

Copy link
Contributor

@kidinov kidinov May 21, 2024

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 and productselector?

Copy link
Contributor Author

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.

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)
) {
val state = productsState.collectAsState()
val gridState = rememberLazyGridState()
LazyVerticalGrid(
columns = GridCells.Adaptive(minSize = 128.dp),
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) {
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 ->
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(
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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. ProductsDataSourceImpl defines an interface with it's public methods anyway

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
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@samiuelson samiuelson May 21, 2024

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.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@samiuelson samiuelson May 21, 2024

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 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 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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

): ProductsDataSource {
return ProductsDataSourceImpl(handler)
}
}
Loading