-
Notifications
You must be signed in to change notification settings - Fork 133
[Compose component] - Add loading param to WCOutlinedButton #14768
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
base: trunk
Are you sure you want to change the base?
Conversation
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
onClick = onClick, | ||
modifier = modifier, | ||
enabled = enabled, | ||
enabled = enabled && !loading, |
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.
Reading the PR description I'm confused with this, I thought you decided to not use this 😕.
I agree with the concern shared in the PR description, so I'm suggesting an alternative in the following patch:
Index: WooCommerce/src/main/kotlin/com/woocommerce/android/ui/compose/component/Buttons.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/compose/component/Buttons.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/compose/component/Buttons.kt
--- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/compose/component/Buttons.kt (revision ef4a400b1f7c7787e7eb4d471f4adc79b3fb0b27)
+++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/compose/component/Buttons.kt (date 1760629171929)
@@ -40,6 +40,8 @@
import androidx.compose.material3.TextButton
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
+import androidx.compose.runtime.getValue
+import androidx.compose.runtime.rememberUpdatedState
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.alpha
@@ -190,10 +192,11 @@
contentPadding: PaddingValues = ButtonDefaults.ContentPadding,
interactionSource: MutableInteractionSource? = null,
) {
+ val loadingState by rememberUpdatedState(loading)
WCOutlinedButton(
- onClick = onClick,
+ onClick = { if (!loadingState) onClick() },
modifier = modifier,
- enabled = enabled && !loading,
+ enabled = enabled,
shape = shape,
colors = colors,
border = border,
@@ -544,6 +547,7 @@
)
}
)
+
}
}
}
WDYT?
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 patch just prevents firing the click event, the click indication (ripple) will still happen, this is not ideal, but I think it's an OK tradeoff if we want to keep the code simple.
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 description was about WCColoredButton
. The code above is for WCOutlinedButton
- I'm not breaking any behavior as this is something new.
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 see, I misunderstood the remark in the PR description, but still I think the remark applies to WCOutlinedButton
too, as disabling the button causes using a different color for the progress indicator:
![]() |
![]() |
For bookings, this is not an issue because we override the content color locally, but anywhere where might want to use the same pattern with WCOutlinedButton
, it will have the gray color, and won't match the app theme.
So I think we should still consider changing this, WDYT?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #14768 +/- ##
============================================
- Coverage 38.30% 38.29% -0.01%
- Complexity 10018 10019 +1
============================================
Files 2118 2118
Lines 118558 118569 +11
Branches 15831 15835 +4
============================================
+ Hits 45409 45411 +2
- Misses 68930 68940 +10
+ Partials 4219 4218 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Version |
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.
Thanks for taking care of this @AdamGrzybkowski, I'm pre-approving, but please check my comment about the enabled = enabled && !loading
.
onClick = onClick, | ||
modifier = modifier, | ||
enabled = enabled, | ||
enabled = enabled && !loading, |
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 see, I misunderstood the remark in the PR description, but still I think the remark applies to WCOutlinedButton
too, as disabling the button causes using a different color for the progress indicator:
![]() |
![]() |
For bookings, this is not an issue because we override the content color locally, but anywhere where might want to use the same pattern with WCOutlinedButton
, it will have the gray color, and won't match the app theme.
So I think we should still consider changing this, WDYT?
Description
As per the request this PR add the
loading
param to the WCOutlinedButton.I didn't do that, because this changes the button appearance in loading state, here's the comparison:
I can do it, but this feels like a bigger decision.
Steps to reproduce
Follow the steps from #14752 to test the button in the app. You can also launch the Preview with all the buttons.
Testing information
The default loading param value is false, so this should be a safe change.
The tests that have been performed
The above.
Images/gif
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.