-
Notifications
You must be signed in to change notification settings - Fork 134
[Bookings] Add list filtering UI #14719
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
[Bookings] Add list filtering UI #14719
Conversation
Generated by 🚫 Danger |
📲 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.
|
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.
@hichamboushaba Since Irfan is AFK do you want to check my comments? I don't think those are blockers and we can update the code in the following PRs.
...erce/src/main/kotlin/com/woocommerce/android/ui/bookings/filter/BookingFilterListFragment.kt
Outdated
Show resolved
Hide resolved
| BookingFilterListScreen( | ||
| state = BookingFilterListViewModel.BookingFilterListUiState( | ||
| items = options.orEmpty(), | ||
| onClose = { requireActivity().onBackPressedDispatcher.onBackPressed() }, |
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.
❓ Shouldn't we use findNavController().popBackStack() for "going back"?
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 catch, Adam! I agree that using popBackStack() and MultiLiveEvent.Event.Exit fits our current architecture better. Thanks for handling it.
I also added a TODO comment to remind us to handle those actions later: 76a2cec
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.
Given this is called by the app bar navigation button (Up button), we should prefer using navigateUp, it doesn't make any difference for this screen since it can't be deeplinked from outside, but just to follow the best practices 🙂
| onClose = { requireActivity().onBackPressedDispatcher.onBackPressed() }, | ||
| onShowBookings = { | ||
| // TODO Pass new filter payload instead of `true` | ||
| navigateBackWithResult(BookingListFragment.BOOKINGS_FILTER_RESULT, true) |
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.
I would consider switching this approach to an in-memory persistence for applied filters exposed as a Flow. This way, we don't have to "return" them. The booking list screen would simply observe them and react to the updates.
@hichamboushaba Thoughts about this?
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.
Some extra thoughts:
One of the cons of this approach would be the extra logic to handle process death. With filters being passed via args we would have this solved by SaveStateHandle for free, with an in-memory filter persistenca that's not the case.
❓ Do we want to persist the filters across app launches?
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 question. I noticed that we persist the latest filters for orders, so I’d say we should do the same for bookings, unless @hichamboushaba disagrees.
If we all agree to persist it, I’d suggest merging this PR first and handling the persistence in a separate one.
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.
oh hi @irfano 👋
If we all agree to persist it, I’d suggest merging this PR first and handling the persistence in a separate one.
Yes, that's obviosly not part of this PR!
I noticed that we persist the latest filters for orders, so I’d say we should do the same for bookings
I noticed it after I asked the question and I agree it would make sense to align.
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.
Sorry folks, I missed the ping about this. I think given it's easy to support persiting filters, we should handle it, we just need to let the iOS folks about this to have a similar feature.
If we add the persistence, I would suggest not returning the filters with navigateBackWithResult, we can just observe them, if we use DataStore for storing the, then we can observe the changes right in the list, and with this we'll have a unique data flow. WDYT?
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.
Also, @irfano go and enjoy your AFK 😄, we'll take it from here for this PR.
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.
If we add the persistence, I would suggest not returning the filters with navigateBackWithResult
Yup, it's already removed in this PR.
we can just observe them, if we use DataStore for storing the, then we can observe the changes right in the list, and with this we'll have a unique data flow. WDYT?
Yes, I already have a working code. Will open a PR soon.
…oking-list-filering-ui
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14719 +/- ##
============================================
- Coverage 38.43% 38.41% -0.02%
- Complexity 9887 9888 +1
============================================
Files 2105 2106 +1
Lines 117088 117138 +50
Branches 15646 15659 +13
============================================
+ Hits 45001 45002 +1
- Misses 67914 67964 +50
+ Partials 4173 4172 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@hichamboushaba Since I've added some code here, do you want to assign yourself and review, or should I approve this myself? :D |
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.
Looks good, nice work.
I've reviewed just your changes @AdamGrzybkowski, given you reviewed the other changes.
| onShowBookings = ::onShowBookings | ||
| ) | ||
| ) | ||
| val uiState = _uiState.asLiveData(viewModelScope.coroutineContext) |
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.
np, is there a reason we are passing a specific coroutine context here? asLiveData already uses Dispatchers.Main.immediate by default, so I don't think this has any effect.
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 catch - I missed it. Removed in 5d3d83c
Part of
Description
This PR adds the filter screen UI, it doesn’t include the filtering logic or sub-screens.
I aimed to align it with the current Order filter screen design.
Steps to reproduce
define( 'WC_BOOKINGS_NEXT_ENABLED', true );The tests that have been performed
Screens below.
Images/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.