Skip to content

Conversation

@AdamGrzybkowski
Copy link
Contributor

@AdamGrzybkowski AdamGrzybkowski commented Oct 8, 2025

Part of WOOMOB-1421

Description

This PR adds support for split pane layout on the bookings tab for bigger screens. The implementation mostly follows the Products tab logic. I highly recommend reviewing commit by commit, where I tried to fix issues one by one.

About the View Order navigation logic.

The OrderDetailFragment has some logic that pretty much makes it impossible to show this Fragment outside the main graph with the Order list in the backstack. I've added an option to ignore it dc55081. Additionally, I've decided to show the Order details in full screen when in Split Pane. While this may not be the best, it's something we already do when opening a Product from Order.

Testing

  1. Launch the app on a tablet or a resizable emulator
  2. Go to the Bookings tab
  3. Play with it

Images/gif

Screen_recording_20251030_153000.mp4
  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Copy link
Contributor

Copilot AI left a 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 PR implements a split-pane layout for the bookings tab on tablet devices, enabling users to view the booking list and details simultaneously. The implementation follows similar patterns used in the Products tab functionality.

  • Added tablet layout support with list-detail split view for larger screens
  • Modified booking navigation to support different display modes (empty state vs showing specific booking)
  • Implemented responsive behavior for configuration changes between single and split-pane layouts

Reviewed Changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
BookingListViewModelTest.kt Added tests for tablet-specific navigation behavior and booking selection
BookingDetailsViewModelTest.kt Updated tests to use new Mode parameter instead of direct bookingId
strings.xml Added "Booking not selected" string for empty state
nav_graph_bookings_details.xml New navigation graph for booking details with Mode argument
nav_graph_bookings.xml Updated to use new details navigation graph
fragment_booking_list.xml Added split-pane layout with list and details containers
TabletLayoutSetupHelper.kt Added configuration flag for automatic layout adjustment
ProductListFragment.kt Updated to support new layout adjustment configuration
DetailsPanePadding.kt New utility for consistent padding in split-pane details
BookingListViewModel.kt Added tablet-specific logic for auto-opening first booking
BookingListFragment.kt Implemented split-pane functionality and configuration change handling
BookingDetailsViewModel.kt Updated to handle empty mode and nullable booking scenarios
BookingDetailsScreen.kt Added empty state UI and split-pane responsive design
BookingDetailsFragment.kt Added Mode sealed class and configuration change handling
BookingsCommunicationViewModel.kt New communication layer between list and details in split-pane mode

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

topBar = {
Toolbar(
title = viewState.toolbarTitle,
navigationIcon = if (context.isTwoPanesShouldBeUsed) null else Icons.AutoMirrored.Filled.ArrowBack,
Copy link

Copilot AI Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider extracting the conditional navigation icon logic into a separate function or computed property to improve readability and maintainability.

Copilot uses AI. Check for mistakes.
@dangermattic
Copy link
Collaborator

dangermattic commented Oct 8, 2025

2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ Class BookingsCommunicationViewModel is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 8, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App NameWooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commitf7963a1
Direct Downloadwoocommerce-wear-prototype-build-pr14720-f7963a1.apk


private fun adjustLayoutForNonTablet(screen: Screen) {
if (screen.twoPanesWereShownBeforeConfigChange) {
if (screen.twoPanesWereShownBeforeConfigChange && screen.automaticallyAdjustLayoutAfterConfigChange) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this logic when switching from split pane to single pane. It shows the detail pane, and we end up with a different navigation stack compared to if we opened the booking details from the list in a single pane from the beginning. This causes some issues with back navigation.

I wanted an option to disable this and handle it separately in the BookingListFragment.

@@ -0,0 +1,25 @@
<?xml version="1.0" encoding="utf-8"?>
<navigation xmlns:android="http://schemas.android.com/apk/res/android"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was separated from the nav_graph_bookings so that I can use this for the details pane.

* The padding mirrors the margin calculations used in TabletLayoutSetupHelper.setDetailsMargins.
*/
@Composable
fun Modifier.detailsPanePadding(): Modifier {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TabletLayoutSetupHelper.setDetailsMargins doesn't work with ComposeView and even if we wrap the ComposeView in a ViewGroup we end up with the whole screen paddes horizontally, including the compose Toolbar. This is not how the details screen look on other tabs, so I've added this modifier to set directly on the booking details content composable.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 8, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App NameWooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commitf7963a1
Direct Downloadwoocommerce-prototype-build-pr14720-f7963a1.apk

@AdamGrzybkowski AdamGrzybkowski force-pushed the issue/WOOMOB-1421_booking_details_slit_pane branch from b8c2ec8 to bc28f65 Compare October 10, 2025 12:28
@AdamGrzybkowski AdamGrzybkowski modified the milestones: 23.5, 23.6 Oct 17, 2025
@AdamGrzybkowski AdamGrzybkowski force-pushed the issue/WOOMOB-1421_booking_details_slit_pane branch from bc28f65 to dc55081 Compare October 30, 2025 14:14

private val booking = bookingsRepository.observeBooking(navArgs.bookingId)
.shareIn(scope = viewModelScope, started = SharingStarted.WhileSubscribed(), replay = 1)
private val bookingId: Long? = (navArgs.mode as? BookingDetailsFragment.Mode.ShowBooking)?.bookingId
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of this. I would like to rework how we model the VM state, but since this PR grew quite a bit, I would rather do it in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A follow up PR is here #14881

@AdamGrzybkowski AdamGrzybkowski marked this pull request as ready for review October 30, 2025 14:39
@codecov-commenter
Copy link

codecov-commenter commented Oct 30, 2025

Codecov Report

❌ Patch coverage is 42.62295% with 35 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.26%. Comparing base (7e5bb1b) to head (f7963a1).
⚠️ Report is 185 commits behind head on trunk.

Files with missing lines Patch % Lines
.../android/ui/compose/modifier/DetailsPanePadding.kt 0.00% 14 Missing ⚠️
...oid/ui/bookings/details/BookingDetailsViewModel.kt 59.25% 1 Missing and 10 partials ⚠️
...roid/ui/bookings/BookingsCommunicationViewModel.kt 0.00% 6 Missing ⚠️
...oid/ui/bookings/details/BookingDetailsViewState.kt 0.00% 0 Missing and 2 partials ⚠️
...e/android/ui/bookings/list/BookingListViewModel.kt 90.90% 0 Missing and 1 partial ⚠️
...oocommerce/android/util/TabletLayoutSetupHelper.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #14720   +/-   ##
=========================================
  Coverage     38.26%   38.26%           
- Complexity    10077    10088   +11     
=========================================
  Files          2131     2133    +2     
  Lines        120744   120788   +44     
  Branches      16547    16559   +12     
=========================================
+ Hits          46206    46225   +19     
- Misses        69840    69859   +19     
- Partials       4698     4704    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a 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 20 out of 20 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


val bookingTask = async {
bookingsRepository.fetchBooking(navArgs.bookingId)
bookingsRepository.fetchBooking(bookingId ?: 0L)
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using 0L as a fallback for bookingId could lead to attempting to fetch a booking with ID 0, which is likely invalid. Since this code is inside a check for navArgs.mode !is BookingDetailsFragment.Mode.ShowBooking that returns early, this line should never execute with a null bookingId. However, the ?: 0L fallback is misleading. Consider using !! with a comment explaining it's guaranteed to be non-null, or better yet, use bookingId directly without the elvis operator since the early return guarantees it's non-null at this point.

Suggested change
bookingsRepository.fetchBooking(bookingId ?: 0L)
// bookingId is guaranteed non-null here due to early return above
bookingsRepository.fetchBooking(bookingId!!)

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, yes - that's what I want to fix as part of this #14720 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed here #14881

Comment on lines 88 to 92
private val contentState = combine(
bookingListHandler.bookingsFlow.map { bookings ->
openFirstLoadedBookingOnTablet(bookings)
with(bookingMapper) { bookings.map { it.toListItem() } }
},
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling openFirstLoadedBookingOnTablet() inside a map operator in a flow means this function will be invoked every time the bookings flow emits, even if bookings haven't changed. This could result in unnecessary event triggers. Consider using distinctUntilChanged() on the bookings flow or adding logic to track whether the first booking has already been auto-opened to prevent duplicate navigation events.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@irfano irfano Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we’re already checking whether selectedBookingIdOnBigScreen == null to prevent duplicate navigation events. (Not a suggestion, just confirming my understanding.)

@wpmobilebot wpmobilebot modified the milestones: 23.6, 23.7 Oct 31, 2025
@wpmobilebot
Copy link
Collaborator

Version 23.6 has now entered code-freeze, so the milestone of this PR has been updated to 23.7.

Copy link
Contributor

@irfano irfano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job! This wasn’t an easy task. I asked a few questions to understand it better, noticed some minor issues, and made a few suggestions.

Padding issue

Screenshot_20251101_002320 After tapping "Add note" and navigating back to the booking details, an unwanted bottom gap appears.

"View customer orders" button issue

Screenshot_20251101_002506 This button doesn’t do anything when the order detail screen is opened from the booking details.

override val detailPaneContainer: View
get() = binding.detailNavContainer
override var twoPanesWereShownBeforeConfigChange: Boolean = false
override val automaticallyAdjustLayoutAfterConfigChange: Boolean = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn’t quite understand why we’re handling configuration changes in Bookings but not in Products. Could you please elaborate on this? Also, how can I test this? I tried it on a tablet, and it always shows two panes in both orientations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made a quick comment mentioning that here #14720 (comment).

We do handle it in the Products tab, but it's fully done in TabletLayoutSetupHelper. There are some problems with this logic, so that's why I wanted to disable that in the Bookings tab.

In short, when you transition from a two-pane to a single-pane layout:

Products
The list pane is hidden, and the details pane gets full width.

Bookings
The list pane gets full width, and we show the Booking details fragment properly by adding it to the main nav graph.

The main difference is that the single-pane navigation stack is fully restored. In addition, the bottom bar visibility is correct.

Here's the recording from the Products tab:

Screen.Recording.2025-11-03.at.11.20.25.mov

You can observe the following:

  1. At 0:16 second mark you can see that the first two pane to single pane transition failed. The product details screen wasn't shown.
  2. Next it worked, but you can see that the bottom bar is visible (it shouldn't)
  3. Then I opened the same details again - notice the bottom bar visibility is different.
  4. Then I navigated back - the previous details pane is visible !?

Here's the Bookings tab for a comparison:

Screen.Recording.2025-11-03.at.11.24.36.mov

To test, you need a resizable emulator or a foldable device big enough to show two panes.

Comment on lines 88 to 92
private val contentState = combine(
bookingListHandler.bookingsFlow.map { bookings ->
openFirstLoadedBookingOnTablet(bookings)
with(bookingMapper) { bookings.map { it.toListItem() } }
},
Copy link
Contributor

@irfano irfano Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we’re already checking whether selectedBookingIdOnBigScreen == null to prevent duplicate navigation events. (Not a suggestion, just confirming my understanding.)

@AdamGrzybkowski
Copy link
Contributor Author

After tapping "Add note" and navigating back to the booking details, an unwanted bottom gap appears.

Yeah, this seems to be a general problem with how we handle insets with edge-to-edge in the app and the combination with Jetpack Compose. This can be reproduced on phones as well, especially when you use the back button from the toolbar. I think it's separate from this PR, so I created an issue for this - WOOMOB-1630

@AdamGrzybkowski
Copy link
Contributor Author

"View customer orders" button issue

Ohh, that's a good catch. In fact, I'm not sure what would be the best way to handle it. This button applies the filter to only match this customer and shows the order list screen... I could hide that button, WDYT?

@irfano
Copy link
Contributor

irfano commented Nov 3, 2025

Another note: We’re adding a color to the selected order item in the list. For consistency, it would be nice to follow the same behavior for bookings as well, but feel free to handle this in a separate PR.

Screenshot_20251103_172329

@irfano
Copy link
Contributor

irfano commented Nov 3, 2025

Ohh, that's a good catch. In fact, I'm not sure what would be the best way to handle it. This button applies the filter to only match this customer and shows the order list screen... I could hide that button, WDYT?

I think hiding the button is the best approach in this case.

@AdamGrzybkowski
Copy link
Contributor Author

Another note: We’re adding a color to the selected order item in the list. For consistency, it would be nice to follow the same behavior for bookings as well, but feel free to handle this in a separate PR.

Good call, I've added an issue for that - WOOMOB-1632

@AdamGrzybkowski
Copy link
Contributor Author

I think hiding the button is the best approach in this case.

Done in f7963a1

I've decided to check whether there's an order list in the backstack or we are in the order detail pane. Otherwise, the button is hidden. This should also work in case the Order screen is shown from other parts of the app that don't support that logic or where it doesn't make sense.

Alternatively, I could use the nav argument ignoreTwoPaneLayoutLogic. This would limit the changes to the scenario you tested, but I think the above is better as it will handle more scenarios.

FYI. This button doesn't work in the current trunk either, so it's great that you found it!

Copy link
Contributor

@irfano irfano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in f7963a1

Nice! It's now hidden when navigating from Bookings.

All good now, thanks for addressing my feedback and for the thorough explanations to my questions. LGTM! 👍🏻

@irfano irfano merged commit 652769f into trunk Nov 4, 2025
18 checks passed
@irfano irfano deleted the issue/WOOMOB-1421_booking_details_slit_pane branch November 4, 2025 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants