-
Notifications
You must be signed in to change notification settings - Fork 131
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
[Woo POS][Design system] Center vertically the input fields in cash and receipts screens #13627
base: trunk
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the layout implementations in two composable screens by replacing the previous ConstraintLayout with a simpler Column-based structure. The changes adjust spacing and alignment by introducing modifiers such as padding, fillMaxWidth, and Arrangement.SpaceBetween, along with Spacer components to manage vertical spacing. The updated layout logic repositions UI elements like text, input fields, error messages, and buttons without altering their overall functionality. Changes
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentScreen.kt (1)
113-122
: Consider using design system spacing consistentlyI notice you're suppressing the
WooPosDesignSystemSpacingUsageRule
and using a fixed64.dp
for the start padding. For better consistency, consider using the design system spacing values throughout.- @Suppress("WooPosDesignSystemSpacingUsageRule") - .padding( - top = WooPosSpacing.XSmall.value, - start = 64.dp - ) + .padding( + top = WooPosSpacing.XSmall.value, + start = WooPosSpacing.XXLarge.value // Assuming there's an appropriate spacing constant + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentScreen.kt
(4 hunks)WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/emailreceipt/WooPosEmailReceiptScreen.kt
(4 hunks)
🔇 Additional comments (8)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/emailreceipt/WooPosEmailReceiptScreen.kt (4)
91-96
: Good layout simplification!Nice refactoring from ConstraintLayout to a simpler Column-based structure. Using
horizontalAlignment = Alignment.CenterHorizontally
ensures that all components are properly aligned and creates a more maintainable layout approach.
97-97
: Excellent vertical centering approachUsing weighted Spacers with
weight(1f)
at both the top and bottom is an elegant way to center the content vertically between the top and bottom of the screen, regardless of screen size or keyboard visibility.Also applies to: 126-126
111-112
: Good modifier usageUsing a direct padding modifier on the input field is cleaner than the previous constraint-based approach, resulting in more readable and maintainable code.
136-138
: Consistent button stylingThe added modifiers for
fillMaxWidth()
and consistent padding improve the button's appearance and ensure it's properly aligned with the design system guidelines.WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentScreen.kt (4)
107-112
: Good layout simplification!Excellent refactoring from ConstraintLayout to a more straightforward Column with
verticalArrangement = Arrangement.SpaceBetween
. This creates a cleaner and more maintainable layout structure.
124-124
: Excellent vertical centering approachUsing weighted Spacers at both the top and bottom effectively centers the input content vertically, ensuring a balanced layout regardless of screen size or keyboard visibility.
Also applies to: 173-173
126-171
: Good component organizationWrapping the input field and related elements in a nested Column with
horizontalAlignment = Alignment.CenterHorizontally
improves the organization of the UI components and ensures proper alignment.
183-188
: Consistent button stylingThe added modifiers for
fillMaxWidth()
and consistent padding through the design system ensures the button maintains proper alignment and appearance.
📲 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.
|
Closes: #13626
Description
The PR change the layouts on cash collection and email screens, making the input fields being centered between the button and the top of the screen. I tried that and it looks good
This comes from this proposal #13615 (review)
Steps to reproduce
The tests that have been performed
Above
Images/gif
02-26--15-35.mp4
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: