[WooPos][Non Simple Product types] Retry UI for pagination errors on variations screens #12972
There are no files selected for viewing
This file contains 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 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 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 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 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 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 |
|---|---|---|
|
|
@@ -16,10 +16,6 @@ import androidx.compose.material.ExperimentalMaterialApi | |
| import androidx.compose.material.Icon | ||
| import androidx.compose.material.IconButton | ||
| import androidx.compose.material.MaterialTheme | ||
| import androidx.compose.material.Text | ||
| import androidx.compose.material.icons.Icons | ||
| import androidx.compose.material.icons.automirrored.filled.ArrowBack | ||
|
|
@@ -29,17 +25,13 @@ import androidx.compose.material.pullrefresh.rememberPullRefreshState | |
| import androidx.compose.runtime.Composable | ||
| import androidx.compose.runtime.LaunchedEffect | ||
| import androidx.compose.runtime.collectAsState | ||
| import androidx.compose.ui.Alignment | ||
| import androidx.compose.ui.Modifier | ||
| import androidx.compose.ui.res.stringResource | ||
| import androidx.compose.ui.text.font.FontWeight | ||
| import androidx.compose.ui.unit.dp | ||
| import androidx.constraintlayout.compose.ConstraintLayout | ||
| import androidx.hilt.navigation.compose.hiltViewModel | ||
| import com.woocommerce.android.R | ||
| import com.woocommerce.android.ui.woopos.common.composeui.WooPosPreview | ||
| import com.woocommerce.android.ui.woopos.common.composeui.WooPosTheme | ||
|
|
@@ -61,27 +53,10 @@ fun WooPosVariationsScreen( | |
| onBackClicked: () -> Unit | ||
| ) { | ||
| val viewModel: WooPosVariationsViewModel = hiltViewModel() | ||
|
|
||
| LaunchedEffect(variableProductData.id) { | ||
| viewModel.init(variableProductData.id) | ||
| } | ||
| val state = viewModel.viewState | ||
| WooPosVariationsScreens( | ||
| modifier, | ||
|
|
@@ -102,7 +77,6 @@ fun WooPosVariationsScreen( | |
| }, | ||
| variableProductData, | ||
| state, | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -118,93 +92,82 @@ private fun WooPosVariationsScreens( | |
| onRetryClicked: () -> Unit, | ||
| variableProductData: VariableProductData, | ||
| state: StateFlow<WooPosVariationsViewState>, | ||
| ) { | ||
| val itemState = state.collectAsState() | ||
| val pullToRefreshState = rememberPullRefreshState( | ||
| itemState.value.reloadingProductsWithPullToRefresh, | ||
| onPullToRefresh | ||
| ) | ||
| Box( | ||
| modifier = modifier | ||
| .fillMaxSize() | ||
| .pullRefresh(pullToRefreshState) | ||
| .padding( | ||
| start = 16.dp.toAdaptivePadding(), | ||
| end = 16.dp.toAdaptivePadding(), | ||
| top = 30.dp.toAdaptivePadding(), | ||
|
There was a problem hiding this comment. I think 30 can not be in 4dp grid. 28 or 32 should be https://m2.material.io/design/layout/spacing-methods.html#baseline-grid |
||
| bottom = 0.dp.toAdaptivePadding(), | ||
| ) | ||
| ) { | ||
| BackHandler(onBack = onBackClicked) | ||
| Column( | ||
| modifier = modifier.fillMaxHeight() | ||
| ) { | ||
| VariationsToolbar( | ||
| variableProductData = variableProductData, | ||
| onBackClicked = onBackClicked | ||
| ) | ||
| when (val itemsState = itemState.value) { | ||
| is WooPosVariationsViewState.Content -> { | ||
| val lazyListState = rememberLazyListState() | ||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| ItemList( | ||
| state = itemsState, | ||
| listState = lazyListState, | ||
| onItemClicked = { | ||
| onItemClicked( | ||
| (it as WooPosItem.Variation).productId, | ||
| it.id | ||
| ) | ||
| }, | ||
| onEndOfProductsListReached = onEndOfItemListReached, | ||
| onErrorWhilePaginating = { | ||
| VariationsError(modifier = Modifier.height(500.dp)) { | ||
| onEndOfItemListReached() | ||
| } | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| is WooPosVariationsViewState.Loading -> ItemsLoadingIndicator( | ||
| minOf(10, variableProductData.numOfVariations) | ||
|
There was a problem hiding this comment. If the goal is to show the exact amount of variations as loading items, then I don't understand why we need to take min from it or 10? There was a problem hiding this comment. The goal is not to show the exact amount of loading items as variations. The goal is to show the exact amount of loading items as variations only if the variation is <= 10. There was a problem hiding this comment. Then, I don't understand the logic behind that. I think we either want to indicate the number of items we load or don't want to do it and just show a generic loading screen similar to the products. Imo this just creates confusion for both us and the users on top of that it leaks logic to the view layer There was a problem hiding this comment. Alright, fixed here: 8c3af25 |
||
| ) | ||
|
|
||
| is WooPosVariationsViewState.Error -> { | ||
| VariationsError(modifier = Modifier.width(640.dp)) { | ||
| onRetryClicked() | ||
| } | ||
| } | ||
|
|
||
| else -> {} | ||
| } | ||
| } | ||
| PullRefreshIndicator( | ||
| modifier = Modifier.align(Alignment.TopCenter), | ||
| refreshing = itemState.value.reloadingProductsWithPullToRefresh, | ||
| state = pullToRefreshState | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| @Composable | ||
| fun VariationsError(modifier: Modifier, onRetryClicked: () -> Unit) { | ||
| Box( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| contentAlignment = Alignment.Center, | ||
| ) { | ||
| WooPosErrorScreen( | ||
| modifier = modifier, | ||
| message = stringResource(id = R.string.woopos_variations_loading_error_title), | ||
| reason = stringResource(id = R.string.woopos_products_loading_error_message), | ||
| primaryButton = Button( | ||
|
|
@@ -316,7 +279,6 @@ fun WooPosVariationsScreenPreview() { | |
| numOfVariations = 20, | ||
| ), | ||
| state = productState, | ||
| ) | ||
| } | ||
| } | ||
This file contains 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 |
|---|---|---|
|
|
@@ -11,11 +11,9 @@ import com.woocommerce.android.ui.woopos.home.items.WooPosVariationsViewState | |
| import com.woocommerce.android.ui.woopos.util.format.WooPosFormatPrice | ||
| import dagger.hilt.android.lifecycle.HiltViewModel | ||
| import kotlinx.coroutines.Job | ||
| import kotlinx.coroutines.flow.MutableStateFlow | ||
| import kotlinx.coroutines.flow.SharingStarted | ||
| import kotlinx.coroutines.flow.StateFlow | ||
| import kotlinx.coroutines.flow.stateIn | ||
| import kotlinx.coroutines.launch | ||
| import javax.inject.Inject | ||
|
|
@@ -37,11 +35,6 @@ class WooPosVariationsViewModel @Inject constructor( | |
| initialValue = _viewState.value, | ||
| ) | ||
|
|
||
| private var fetchJob: Job? = null | ||
| private var loadMoreJob: Job? = null | ||
|
|
||
|
|
@@ -100,18 +93,15 @@ class WooPosVariationsViewModel @Inject constructor( | |
| if (!variationsDataSource.canLoadMore()) { | ||
| return | ||
| } | ||
| _viewState.value = currentState.copy(loadingMore = true, errorLoadingMoreItems = false) | ||
| loadMoreJob?.cancel() | ||
| loadMoreJob = viewModelScope.launch { | ||
| val result = variationsDataSource.loadMore(productId) | ||
| if (result.isSuccess) { | ||
|
There was a problem hiding this comment. np: |
||
| Result.success(Unit) | ||
|
There was a problem hiding this comment. This is no longer relevant as result of this change: a45bd6c |
||
|
There was a problem hiding this comment. Can you use currentState here? There was a problem hiding this comment. This is no longer relevant as result of this change: a45bd6c |
||
| _viewState.value = currentState.copy(loadingMore = false, errorLoadingMoreItems = false) | ||
| } else { | ||
| _viewState.value = currentState.copy(loadingMore = false, errorLoadingMoreItems = true) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -151,8 +141,4 @@ class WooPosVariationsViewModel @Inject constructor( | |
| private fun onEndOfVariationsListReached(productId: Long) { | ||
| loadMore(productId) | ||
| } | ||
| } | ||
This file contains 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
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.
The state defined like that can contain invalidate states, e.g.
loadingMore = trueanderrorLoadingMoreItems = true.We probably should do something like
By the way,
ContentViewStatedoes not follow the naming convention of the top-level classes. It probably should beWooPosItemsViewState.And there are a few more, e.g.,ToolbarAccessibilityLabels, for instance. And some classes that are in theitemspackage now, e.g.,WooPosBannershould either contain an item in them or be placed in another packageThere 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 this. I had this on my list to refactor hence I kept the PR as draft. Here's the refactored state: a45bd6c