Skip to content

Conversation

@AdamGrzybkowski
Copy link
Contributor

WOOMOB-1484

Description

While fixing a bug related to the Trash product button visibility (#14657) I introduced this bug where the trash button is not visible in the split pane layout.

This PR adds an additional check to make sure it's enabled in split pane even if the product list is not in the backstack.

Steps to reproduce

  1. Open the app on tablet
  2. Go to products
  3. Tap on the ... menu button
  4. Verify the Trash product is visible

Testing information

The above plus:

Using a tablet
2. Go to order
3. Tap on a product
4. Verify the Trash product is gone

Using a phone

  1. Go to products
  2. Tap on a product
  3. Tap on the ... menu button
  4. Verify the Trash product is visible

The tests that have been performed

The above

Images/gif

Before After
trash_unavailable trash_visible
  • 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.

@AdamGrzybkowski AdamGrzybkowski added this to the 23.5 milestone Oct 10, 2025
@AdamGrzybkowski AdamGrzybkowski added category: tablet Specific to tablet devices such as a Galaxy Tab or an iPad. Bug labels Oct 10, 2025
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 fixes a bug where the "Trash product" button was not visible in split pane layout on tablets. The fix adds logic to check if the product detail view is displayed in a detail pane, enabling the trash button even when the product list is not in the backstack.

  • Updated the trash button visibility logic to account for tablet split pane layout
  • Added a check for detail pane context to enable trash functionality in two-pane mode

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

val productListInBackstack =
findNavController().previousBackStackEntry?.destination?.id == BottomNavigationPosition.PRODUCTS.id
val isInDetailPane =
requireContext().isTwoPanesShouldBeUsed && (parentFragment?.id == R.id.detail_nav_container)
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

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

Consider using safe call operator for parentFragment property access. While the logic may be correct in this context, using parentFragment?.id followed by direct comparison could potentially lead to unexpected behavior if parentFragment is null. Consider parentFragment?.id == R.id.detail_nav_container ?: false or restructure the condition to be more explicit about null handling.

Suggested change
requireContext().isTwoPanesShouldBeUsed && (parentFragment?.id == R.id.detail_nav_container)
requireContext().isTwoPanesShouldBeUsed && ((parentFragment?.id == R.id.detail_nav_container) ?: false)

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

wpmobilebot commented Oct 10, 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
Commit8bb2dbd
Direct Downloadwoocommerce-wear-prototype-build-pr14734-8bb2dbd.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 10, 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
Commit8bb2dbd
Direct Downloadwoocommerce-prototype-build-pr14734-8bb2dbd.apk

Copy link
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for working on this @AdamGrzybkowski, and sorry for missing it too in my review of the previous PR 😅

@AdamGrzybkowski AdamGrzybkowski force-pushed the issue/WOOMOB-1484_trash_product_in_split_view branch from 50cc233 to 8bb2dbd Compare October 10, 2025 20:15
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 38.43%. Comparing base (01217d7) to head (8bb2dbd).

Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #14734   +/-   ##
=========================================
  Coverage     38.43%   38.43%           
- Complexity     9895     9896    +1     
=========================================
  Files          2109     2109           
  Lines        117106   117106           
  Branches      15657    15657           
=========================================
+ Hits          45013    45015    +2     
+ Misses        67921    67920    -1     
+ Partials       4172     4171    -1     

☔ 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.

@AdamGrzybkowski AdamGrzybkowski merged commit cd12bb0 into trunk Oct 13, 2025
16 checks passed
@AdamGrzybkowski AdamGrzybkowski deleted the issue/WOOMOB-1484_trash_product_in_split_view branch October 13, 2025 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug category: tablet Specific to tablet devices such as a Galaxy Tab or an iPad.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants