-
Notifications
You must be signed in to change notification settings - Fork 135
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
[WooPos][Non Simple Product types] Retry UI for pagination errors on variations screens #12972
base: trunk
Are you sure you want to change the base?
Changes from 27 commits
bddcae5
9873533
42f4e1a
0fe310a
d84a3a8
455d396
30b6d5d
53a1838
bab7cbe
8ceaa5f
e5f7823
a45bd6c
71ee602
84febe1
5b60693
20d14bd
9685cd2
0bd442d
e40c789
e94d0e5
3ed43d3
187dfcd
6fe46f2
0f5cf15
d4d0060
81cb158
d03d6a2
28584e8
aa38f10
6a3b777
6bb0870
19a7ed5
ebbf621
623125f
a9780a3
45a1d7e
ba90251
4a920cb
8c3af25
1eb6c32
ef53ce9
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 |
---|---|---|
@@ -0,0 +1,112 @@ | ||
package com.woocommerce.android.ui.woopos.common.composeui.component | ||
|
||
import androidx.compose.foundation.layout.Arrangement | ||
import androidx.compose.foundation.layout.Row | ||
import androidx.compose.foundation.layout.Spacer | ||
import androidx.compose.foundation.layout.fillMaxWidth | ||
import androidx.compose.foundation.layout.height | ||
import androidx.compose.foundation.layout.padding | ||
import androidx.compose.foundation.layout.size | ||
import androidx.compose.foundation.layout.width | ||
import androidx.compose.foundation.shape.RoundedCornerShape | ||
import androidx.compose.material.Icon | ||
import androidx.compose.material.MaterialTheme | ||
import androidx.compose.material.Text | ||
import androidx.compose.runtime.Composable | ||
import androidx.compose.ui.Alignment | ||
import androidx.compose.ui.Modifier | ||
import androidx.compose.ui.draw.clip | ||
import androidx.compose.ui.graphics.Color | ||
import androidx.compose.ui.graphics.painter.Painter | ||
import androidx.compose.ui.res.painterResource | ||
import androidx.compose.ui.res.stringResource | ||
import androidx.compose.ui.text.font.FontWeight | ||
import androidx.compose.ui.unit.dp | ||
import com.woocommerce.android.R | ||
import com.woocommerce.android.ui.woopos.common.composeui.WooPosCard | ||
import com.woocommerce.android.ui.woopos.common.composeui.WooPosPreview | ||
import com.woocommerce.android.ui.woopos.common.composeui.WooPosTheme | ||
import com.woocommerce.android.ui.woopos.common.composeui.toAdaptivePadding | ||
|
||
@Composable | ||
fun WooPosPaginationErrorScreen( | ||
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. np: maybe call it somehow closer to reality? 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. Done: aa38f10 |
||
modifier: Modifier = Modifier, | ||
icon: Painter = painterResource(id = R.drawable.woo_pos_ic_error), | ||
message: String, | ||
primaryButton: Button, | ||
) { | ||
WooPosCard { | ||
WooPosPaginationErrorScreenContent( | ||
modifier = modifier, | ||
icon = icon, | ||
message = message, | ||
primaryButton = primaryButton | ||
) | ||
} | ||
} | ||
|
||
@Composable | ||
private fun WooPosPaginationErrorScreenContent( | ||
modifier: Modifier, | ||
icon: Painter, | ||
message: String, | ||
primaryButton: Button | ||
) { | ||
Row( | ||
modifier = modifier | ||
.fillMaxWidth() | ||
.height(112.dp) | ||
.clip(RoundedCornerShape(16.dp)) | ||
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. If the goal was to mimic the product's items, then it doesn't match. Please take a look at Btw, in 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. Done: 19a7ed5 |
||
.padding(16.dp.toAdaptivePadding()), | ||
verticalAlignment = Alignment.CenterVertically, | ||
horizontalArrangement = Arrangement.SpaceBetween | ||
) { | ||
Row( | ||
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. Why do you need 3 rows nested here 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. Refactored here: 19a7ed5 |
||
verticalAlignment = Alignment.CenterVertically, | ||
horizontalArrangement = Arrangement.spacedBy(8.dp), | ||
modifier = Modifier.weight(1f) | ||
) { | ||
Icon( | ||
modifier = Modifier.size(24.dp), | ||
painter = icon, | ||
contentDescription = stringResource(R.string.woopos_error_icon_content_description), | ||
tint = Color.Unspecified, | ||
) | ||
|
||
Spacer(modifier = Modifier.width(24.dp.toAdaptivePadding())) | ||
|
||
Text( | ||
text = message, | ||
style = MaterialTheme.typography.h5, | ||
fontWeight = FontWeight.SemiBold, | ||
color = MaterialTheme.colors.error | ||
) | ||
} | ||
|
||
Row( | ||
modifier = Modifier.weight(0.5f) | ||
) { | ||
WooPosButton( | ||
text = primaryButton.text, | ||
onClick = primaryButton.click, | ||
modifier = Modifier | ||
.height(50.dp) | ||
.clip(RoundedCornerShape(16.dp)) | ||
) | ||
} | ||
} | ||
} | ||
|
||
@Composable | ||
@WooPosPreview | ||
fun WooPosPaginationErrorScreenPreview() { | ||
WooPosTheme { | ||
WooPosPaginationErrorScreen( | ||
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 am considering whether a preview with a list or a list with outer padding would be better for development and debugging. 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. We already have a preview like this in 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 find it a matter of personal preference, but switching between 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. Done: 1eb6c32 |
||
message = "Error loading products", | ||
primaryButton = Button( | ||
text = "Load more", | ||
click = {} | ||
) | ||
) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,12 @@ sealed class WooPosBaseViewState( | |
|
||
interface ContentViewState { | ||
val items: List<WooPosItem> | ||
val loadingMore: Boolean | ||
val reloadingProductsWithPullToRefresh: Boolean | ||
val paginationState: PaginationState | ||
} | ||
|
||
sealed interface PaginationState { | ||
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. Just curious, why interface? 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. Just for flexibility as this paginationState will be used for different kind of items in future and I wasn't sure if i want to tie up the subtype into a restricted base class. At this point. It doesn't make any difference whether it is a sealed class or interface. 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.
Imo sealed interface has to be used on purpose, and if there is no purpose, then it's better to use class to avoid confusion 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. Done: a9780a3 |
||
data object None : PaginationState | ||
data object Loading : PaginationState | ||
data object Error : PaginationState | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,7 @@ import com.woocommerce.android.ui.woopos.common.composeui.WooPosPreview | |
import com.woocommerce.android.ui.woopos.common.composeui.WooPosTheme | ||
import com.woocommerce.android.ui.woopos.common.composeui.component.Button | ||
import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosErrorScreen | ||
import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosPaginationErrorScreen | ||
import com.woocommerce.android.ui.woopos.common.composeui.toAdaptivePadding | ||
import com.woocommerce.android.ui.woopos.home.items.WooPosItem.SimpleProduct | ||
import com.woocommerce.android.ui.woopos.home.items.WooPosItem.VariableProduct | ||
|
@@ -169,7 +170,13 @@ private fun MainItemsList( | |
listState, | ||
onItemClicked, | ||
onEndOfItemListReached, | ||
) | ||
) { | ||
ProductsPaginationError( | ||
onRetryClicked = { | ||
onEndOfItemListReached() | ||
} | ||
) | ||
} | ||
} | ||
} | ||
|
||
|
@@ -317,6 +324,17 @@ fun ProductsError(onRetryClicked: () -> Unit) { | |
} | ||
} | ||
|
||
@Composable | ||
fun ProductsPaginationError(onRetryClicked: () -> Unit) { | ||
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. if it's public then it should be called with 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. And in other places the same 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. Done: 6a3b777 |
||
WooPosPaginationErrorScreen( | ||
message = stringResource(id = R.string.woopos_items_pagination_error), | ||
primaryButton = Button( | ||
text = stringResource(id = R.string.woopos_items_pagination_load_more_label), | ||
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. Not sure if it's missed but in both places it's still
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. Done: 623125f |
||
click = onRetryClicked | ||
), | ||
) | ||
} | ||
|
||
@OptIn(ExperimentalMaterialApi::class) | ||
@Composable | ||
@WooPosPreview | ||
|
@@ -353,7 +371,7 @@ fun WooPosItemsScreenPreview(modifier: Modifier = Modifier) { | |
imageUrl = null, | ||
), | ||
), | ||
loadingMore = true, | ||
paginationState = PaginationState.Loading, | ||
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. Shell we also have preview for the pagination error? 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. Done: 6bb0870 |
||
reloadingProductsWithPullToRefresh = true, | ||
bannerState = WooPosItemsViewState.Content.BannerState( | ||
isBannerHiddenByUser = true, | ||
|
@@ -472,7 +490,6 @@ fun WooPosHomeScreenItemsWithSimpleProductsOnlyBannerPreview() { | |
imageUrl = null, | ||
), | ||
), | ||
loadingMore = false, | ||
reloadingProductsWithPullToRefresh = true, | ||
bannerState = WooPosItemsViewState.Content.BannerState( | ||
isBannerHiddenByUser = false, | ||
|
@@ -525,7 +542,6 @@ fun WooPosHomeScreenItemsWithInfoIconInToolbarPreview() { | |
imageUrl = null, | ||
), | ||
), | ||
loadingMore = false, | ||
reloadingProductsWithPullToRefresh = false, | ||
bannerState = WooPosItemsViewState.Content.BannerState( | ||
isBannerHiddenByUser = true, | ||
|
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.
Just noticed that in many places "toAdaptivePaddings" are not used, so the layout will look bad on different screen types
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.
Fixed. Let me know if I have missed anywhere else - e94d0e5