-
Notifications
You must be signed in to change notification settings - Fork 135
[CIAB Bookings] Update filter screen navigation to use Compose Navigation #14859
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
[CIAB Bookings] Update filter screen navigation to use Compose Navigation #14859
Conversation
📲 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.
|
272fd25 to
126a1fa
Compare
AdamGrzybkowski
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.
Great work! I like it!
For communication between the parent ViewModel (BookingFilterListViewModel) and other ViewModels, we'll use simple callbacks passed to the subscreen Composables, which then pass them back to their ViewModels. Assisted injection improves this solution, as shown in this commit. This approach eliminates the need to rely on events for communication.
I think that's a good idea, I'm okay with events too. One thing to keep in mind is that we will likely need to update the Root ViewModel multiple times without closing the screen, with the multiselect options.
| composable(BookingFilterPage.List.route) { | ||
| BookingFilterRootPage(state.items) | ||
| } |
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.
| composable(BookingFilterPage.List.route) { | |
| BookingFilterRootPage(state.items) | |
| } | |
| composable<BookingFilterPage.List> { | |
| BookingFilterRootPage(state.items) | |
| } |
np: We could use the type-safe destinations to build the graph. We need to make those serializable, but I don't think it's a problem here.
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.
Yes, I thought about it, but given we don't use the serialization plugin on the app, I didn't use, and given we most likely won't need it as the state already exposes all the information that we need, I decided to just use String routes, and they will be static routes.
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.
Related to this, I thought about converting BookingFilterPage to an enum, but I'm not sure if we'll need to pass data for the items in the future, I think given the root ViewModel state is available, we probably won't need any addiitonal data on the currentPage property, WDYT?
If we use an item, we can then just use the name property for route, but it's a minor point.
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.
Good call. At the moment, I can't think of any data we would pass so I think we should be okay with an enum!
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.
Done in 6474dc8, I kept using Camel Case for the item names, and kept the route extension, this way even if we need to use a sealed inteface, it will be an easy change.
Good point, but I think it's something to discuss later: If we need to follow what you suggested, then we can use two callbacks. But we may not need it if we follow the same approach we use in Order filters today, we save the changes only when the user either taps on "Show orders" or back button, which means we'll close the screen anyway when updating the root ViewModel. |
|
Correct, though the |
Yes, later after I sent the comment I thought about this and figured my mistake, you are right, we'll need two callbacks then. Thanks @AdamGrzybkowski |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #14859 +/- ##
============================================
- Coverage 38.27% 38.27% -0.01%
+ Complexity 10089 10082 -7
============================================
Files 2132 2132
Lines 120771 120772 +1
Branches 16568 16553 -15
============================================
Hits 46224 46224
Misses 69850 69850
- Partials 4697 4698 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes WOOMOB-1608
Description
As discussed and agreed, this PR updates the filters internal navigation to use Compose Navigation, the navigation is still driven by the state of the
ViewModel, the addedNavHostand its graph will just reflect the selected page.As stated, the benefit of this solution is that we'll have a correct lifecycle for the sub-ViewModels we use for the filter subscreens, as the navigation component will use a proper
ViewModelStoretied to the composable destination.To better test the solution, I rebased the draft customer filter PR on this branch, and this shows how we can instantiate the sub-ViewModels, and how we can navigate back.
For communication between the parent ViewModel (
BookingFilterListViewModel) and other ViewModels, we'll use simple callbacks passed to the subscreen Composables, which then pass them back to their ViewModels. Assisted injection improves this solution, as shown in this commit. This approach eliminates the need to rely on events for communication.Please share your thoughts on the suggested approach.
Test Steps
Important: For step 4, make sure that you select an actual customer, and not a guest, tapping on guests won't have any effect.
Images/gif
Screen_recording_20251029_185420.mp4
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.