Skip to content
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

[Bulk Update Orders] Enable multiple selection in Order List: Part II #13151

Merged
merged 16 commits into from
Dec 27, 2024

Conversation

hafizrahman
Copy link
Contributor

@hafizrahman hafizrahman commented Dec 17, 2024

Part of: #13130, #13150

please do not merge until target is trunk

Description

This PR adds various UI-related updates to Orders list screen, related to the bulk update feature. Those updates are:

  1. Adds a checkbox showing when an Order is selected in multi select mode, similar to how it works in Products screen.
  2. Hides bottom nav bar when multi selection mode is active, similar to behavior on Products.
  3. Disables filter button when selection mode is active, similar to behavior on Products.
  4. Disables pull to refresh when selection mode is active, similar to behavior on Products.
  5. Hides Create Order FAB when selection mode is active, similar to behavior on Products.
  6. Disables navigation to Order details on edge cases, e.g: when using Talkback

Steps to reproduce

  1. Build app, go to Orders,
  2. Long press an Order to enable multiple selection, ensure it shows the checkbox image next to it.
  3. Play around with selecting/deselecting other Orders.
  4. Ensure that bottom nav bar is hidden when multiple selection is active.
  5. Ensure "Filter" button is inactive when multiple selection is active.
  6. Ensure pull to refresh can't be done when multiple selection is active.
  7. Ensure Create Order FAB is hidden when multiple selection is active.

Testing information

Test this both in phone and tablet (2 panel) mode, the test items should apply to both.

The tests that have been performed

I've tested the steps above both on phone and tablet simulator, API 32 and 34.

Images/gif

Screen.Recording.2024-12-18.at.11.25.24.mov
  • 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.

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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 17, 2024

📲 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 Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit96c8072
Direct Downloadwoocommerce-wear-prototype-build-pr13151-96c8072.apk

This requires some refactoring on how isSearching is stored, since hiding the bottom bar has to check for both searching and selecting state.
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 17, 2024

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

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit96c8072
Direct Downloadwoocommerce-prototype-build-pr13151-96c8072.apk

@hafizrahman hafizrahman added status: do not merge Dependent on another PR, ready for review but not ready for merge. feature: order list Related to the order list. type: task An internally driven task. labels Dec 18, 2024
@hafizrahman hafizrahman added this to the 21.3 milestone Dec 18, 2024
@hafizrahman hafizrahman marked this pull request as ready for review December 18, 2024 04:29
@irfano irfano self-assigned this Dec 19, 2024
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.

Before starting to review code, I want to list issues I found while testing:

  • In selection mode, when tapped the create order button, the title remains "`order selected"
Video
new-order.webm
  • In selection mode, when changing the orientation, the title remains "`order selected".
Video
orientation.webm
  • When Talkback is enabled, we can select the order with double tap but holding the second tap. In selection mode, I am able to open an order. I think I shouldn't be able to do it, as I can't do with the product list.
Video
talkback.webm

Base automatically changed from task/13130-enable-order-multi-select to trunk December 19, 2024 23:10
@hafizrahman hafizrahman removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Dec 20, 2024
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 8 lines in your changes missing coverage. Please review.

Project coverage is 40.52%. Comparing base (8ec2efc) to head (96c8072).
Report is 156 commits behind head on trunk.

Files with missing lines Patch % Lines
...merce/android/ui/orders/list/OrderListViewModel.kt 50.00% 5 Missing and 1 partial ⚠️
...erce/android/ui/orders/filters/OrderFiltersCard.kt 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #13151   +/-   ##
=========================================
  Coverage     40.52%   40.52%           
- Complexity     6357     6358    +1     
=========================================
  Files          1343     1343           
  Lines         77165    77175   +10     
  Branches      10592    10593    +1     
=========================================
+ Hits          31269    31274    +5     
- Misses        43152    43156    +4     
- Partials       2744     2745    +1     

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

@hafizrahman
Copy link
Contributor Author

hafizrahman commented Dec 20, 2024

Thanks for the check @irfano !

In selection mode, when tapped the create order button, the title remains "`order selected"

Like in Product, the create order button should be hidden in selection mode. Added in 2976ce6

In selection mode, when changing the orientation, the title remains "`order selected".

I haven't worked on persisting selection state during configuration change yet as it's a bit non-trivial. If you see there, the selected products are lost, too. I will work on this as a separate PR, the task is already tracked in #13130

When Talkback is enabled, we can select the order with double tap but holding the second tap. In selection mode, I am able to open an order. I think I shouldn't be able to do it, as I can't do with the product list.

I added a commit on 59aa27e to deal with this, it's a similar fix as Products in #7971

@hafizrahman hafizrahman modified the milestones: 21.3, 21.4 Dec 20, 2024
@hafizrahman hafizrahman requested a review from irfano December 20, 2024 01:26
@hafizrahman
Copy link
Contributor Author

hafizrahman commented Dec 24, 2024

@irfano

Thanks for your design reviews!

Do we have a Figma design for this checkbox?

The constraint for this project is that we don't have any specific design, we just want to follow Products list. As I worked on this, though, I noticed that there are differences between the two screens, so replicating it exactly is not possible. For example, Products screen always has a thumbnail area, which gets replaced by the checkbox in selection mode. Orders screen doesn't have this, so the checkbox will disrupt the layout more in comparison. This affects smaller things like divider lines, etc, which you noticed in the review.

As there's no specific design direction, I just opt to work on something that makes sense visually. I added a refactor commit bb014fd that incorporates your feedback, while also tries to make it more similar to Products. These includes:

  • Checkbox size now matches the size in Products
  • Checkbox horizontal spacing now matches the size in Products
  • Checkbox is now vertically centered
  • Order item divider now retains the original horizontal spacing at the start
  • Checkbox now has no transparency because it makes no sense when there's nothing below it, unlike in Products

This is how it looks like now:

I think this would be a good starting point for further design discussion, so if you have more feedbacks, I'd love to hear it. Since we don't have specific design to follow, I think we can aim for something that is essentially usable and not confusing to merchants.

@hafizrahman hafizrahman requested a review from irfano December 24, 2024 05:40
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.

Thanks for explaining everything and sharing screenshots for edge cases, @hafizrahman! LGTM! I've just added a minor feedback. I believe we're good to merge after that. I’m pre-approving it. Great work! 🚀

WooCommerce/src/main/res/layout/order_list_item.xml Outdated Show resolved Hide resolved
WooCommerce/src/main/res/layout/order_list_item.xml Outdated Show resolved Hide resolved
@hafizrahman hafizrahman merged commit 16cc17a into trunk Dec 27, 2024
14 of 15 checks passed
@hafizrahman hafizrahman deleted the task/13130-order-multi-select-ii branch December 27, 2024 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: order list Related to the order list. type: task An internally driven task.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants