-
Notifications
You must be signed in to change notification settings - Fork 136
[Woo POS] Fix status bar height handling in Orders Loading and Order Details screen #15023
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
- Add statusBarsPadding to the root Row to respect system windows. - Remove top padding from WooPosOrderDetailsLoadingPane modifier. - Update padding in WooPosOrderDetailsLoadingPane to be horizontal-only. - Add a Spacer and vertical padding for better visual alignment in the details pane.
… height - Add `statusBarsPadding()` to the main column modifier to prevent overlap with system bars. - Set a minimum height for the toolbar row using `WOO_POS_ORDERS_TOOLBAR_HEIGHT`. - Align toolbar content vertically to the center.
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.
Pull request overview
This pull request fixes status bar height handling in the WooCommerce POS Orders Loading and Order Details screens by applying proper status bar padding to ensure UI elements are not obscured by the system status bar.
- Renamed
WooPosOrdersLoadingStatetoWooPosOrdersLoadingScreenfor consistency - Applied
statusBarsPadding()modifier to both loading and detail screens to account for status bar height - Adjusted padding and alignment in order detail views to maintain proper spacing with the new status bar handling
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| WooPosOrdersScreen.kt | Updated function call to use renamed WooPosOrdersLoadingScreen() |
| WooPosOrdersLoadingScreen.kt | Added status bar padding to main layout, adjusted internal padding to horizontal-only, and refined spacing in detail pane |
| WooPosOrdersDetails.kt | Added status bar padding to scrollable column and set minimum height for header row to match toolbar height |
Comments suppressed due to low confidence (1)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersLoadingScreen.kt:169
- The
paddingmodifier is applied afterclip, which means it will reduce the shimmer box size rather than add spacing around it. The modifiers should be reordered if you want to add spacing:
.clip(RoundedCornerShape(WooPosCornerRadius.Small.value))Remove the .padding(vertical = WooPosSpacing.Small.value) line or move it before .clip() if padding is needed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
| import com.woocommerce.android.ui.woopos.common.composeui.designsystem.WooPosTypography | ||
|
|
||
| @Composable | ||
| fun WooPosOrdersLoadingState() { |
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.
WooPosOrdersLoadingState was a bit misleading, because it fits more of a name for a UI state model.
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.
Screen is also misleading, as this is not a screen but one of the "states" (loading) of the OrdersScreen
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
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.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersLoadingScreen.kt:169
- The
.padding(vertical = WooPosSpacing.Small.value)modifier is applied after.clip(). In Compose, modifier order matters - padding applied after clipping won't be clipped and may not produce the intended visual effect. Consider moving the padding before the clip modifier if the padding should be part of the clipped area, or verify this ordering is intentional.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/orders/WooPosOrdersDetails.kt
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #15023 +/- ##
============================================
+ Coverage 38.54% 38.59% +0.05%
+ Complexity 10293 10292 -1
============================================
Files 2161 2160 -1
Lines 122680 122502 -178
Branches 16910 16887 -23
============================================
- Hits 47287 47284 -3
+ Misses 70593 70418 -175
Partials 4800 4800 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Row( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .statusBarsPadding() |
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.
Shell we have it on the top level WooPosOrdersScreen? Why only 2 states not the rest then?
kidinov
left a comment
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.
@samiuelson I think it's not the best handling of this.
- Empty and error states not handled
- Loading state has the layout broken - notice the "list" part has padding at the top when loading.
…ove redundant paddings Refactor the layout structure in WooPosOrdersScreen to apply `statusBarsPadding()` at the root Box level instead of on individual child components like the search bar. Remove redundant `statusBarsPadding` and adjust padding values in `WooPosOrdersDetails` and `WooPosOrdersLoadingScreen` to align with this change.
Thanks for the review and suggestions, @kidinov. I applied padding to the root Box and updated the paddings in the children. Take a look: |
kidinov
left a comment
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.
- Remove global `statusBarsPadding` from the root `WooPosOrdersScreen` container. - Apply `statusBarsPadding` individually to `OrdersContent`, `OrdersEmpty`, `OrdersError`, and `WooPosToolbar`. - Add `statusBarsPadding` to `WooPosOrdersDetails`. - Refactor `WooPosOrdersLoadingScreen` layout to correctly handle status bar insets and toolbar spacing.
Ah, I didn't realize you meant the backgrounds of the panes are not shown behind the status bar, and I didn't notice that either. To accomplish that, we can't use a single padding at the root Box element, but we need to adjust the paddings in all the children. This is fixed in: 92be6f7
|
|
Version |







WOOMOB-1809
Description
This pull request fixes status bar height handling in the WooCommerce POS Orders Loading and Order Details screens by applying proper status bar padding to ensure UI elements are not obscured by the system status bar.
Test Steps
Verify POS Orders Loading and Content state have correct UI.
Images/gif
Loading
Content
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.