-
Notifications
You must be signed in to change notification settings - Fork 131
[WOOMOB-192] Update Empty Coupon Screen #14014
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
Changes from all commits
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 |
---|---|---|
|
@@ -50,6 +50,7 @@ import coil.request.ImageRequest | |
import com.woocommerce.android.R | ||
import com.woocommerce.android.ui.woopos.common.composeui.WooPosPreview | ||
import com.woocommerce.android.ui.woopos.common.composeui.component.ShadowType | ||
import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosButton | ||
import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosCard | ||
import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosLazyColumn | ||
import com.woocommerce.android.ui.woopos.common.composeui.component.WooPosShimmerBox | ||
|
@@ -478,6 +479,44 @@ fun WooPosItemsEmptyList( | |
title: String, | ||
message: String, | ||
contentDescription: String, | ||
) { | ||
WooPosItemsEmptyListInternal( | ||
modifier = modifier, | ||
title = title, | ||
message = message, | ||
contentDescription = contentDescription, | ||
actionLabel = null, | ||
onActionClicked = null | ||
) | ||
} | ||
|
||
@Composable | ||
fun WooPosItemsEmptyList( | ||
modifier: Modifier = Modifier, | ||
title: String, | ||
message: String, | ||
contentDescription: String, | ||
actionLabel: String, | ||
onActionClicked: (() -> Unit), | ||
) { | ||
WooPosItemsEmptyListInternal( | ||
modifier = modifier, | ||
title = title, | ||
message = message, | ||
contentDescription = contentDescription, | ||
actionLabel = actionLabel, | ||
onActionClicked = onActionClicked | ||
) | ||
} | ||
|
||
@Composable | ||
private fun WooPosItemsEmptyListInternal( | ||
modifier: Modifier = Modifier, | ||
title: String, | ||
message: String, | ||
contentDescription: String, | ||
actionLabel: String?, | ||
onActionClicked: (() -> Unit)? = null, | ||
) { | ||
Box( | ||
modifier = modifier.verticalScroll(rememberScrollState()), | ||
|
@@ -510,7 +549,17 @@ fun WooPosItemsEmptyList( | |
textAlign = TextAlign.Center | ||
) | ||
|
||
Spacer(modifier = Modifier.height(WooPosSpacing.Medium.value.toAdaptivePadding())) | ||
Spacer(modifier = Modifier.height(WooPosSpacing.XLarge.value.toAdaptivePadding())) | ||
|
||
if (onActionClicked != null && actionLabel != null) { | ||
WooPosButton( | ||
text = actionLabel, | ||
onClick = onActionClicked, | ||
modifier = Modifier | ||
.fillMaxWidth(0.5f) | ||
.height(80.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. 💡n.p.: Text size in the button can be scaled up in the accessibility settings. Maybe let's use top padding instead of fixed height 🤔 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. Hmm, I see what you mean and I tend to agree, however, this was copied and there are 5+ other buttons like this in the POS. Considering we tested other projects for accessibility and didn't report this an issue I believe it might look fine even with the biggest font. At the moment, it's imo not worth the effort to diving more into this. |
||
) | ||
} | ||
} | ||
} | ||
} | ||
|
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've considered creating a sealed class to ensure
actionLabel
andonActionClicked
either need to be both null or both non-null, but in the end I opted for making this function private and includeInternal
in its name instead and provide two overloads. Let me know what you think.