-
Notifications
You must be signed in to change notification settings - Fork 135
[Bookings] Type filter #14858
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] Type filter #14858
Conversation
Moves the `BookingFilterListItem` data class from `BookingFilterListUiState.kt` to a new, dedicated file `BookingFilterListItem.kt` within the same package.
This commit introduces a new Composable, `SingleChoiceFilterPage`, for displaying a list of filter options where only one can be selected at a time.
Introduces the `BookingTypeFilterPage` composable. This page utilizes the generic `SingleChoiceFilterPage` to display a list of booking type filter options.
Generated by 🚫 Danger |
| data class BookingFilterListItem( | ||
| @StringRes val title: Int, | ||
| val value: String? = null, | ||
| val onClick: () -> Unit = {} | ||
| ) |
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 extracted this from BookingFilterListUiState to make it reusable across all screens, not just the root page.
📲 You can test the changes from this Pull Request in WooCommerce-Wear 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.
I tried to use a structure similar to BookingFilterListUiState here.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14858 +/- ##
============================================
- Coverage 38.26% 38.24% -0.03%
Complexity 10077 10077
============================================
Files 2131 2135 +4
Lines 120744 120810 +66
Branches 16547 16561 +14
============================================
Hits 46206 46206
- Misses 69840 69906 +66
Partials 4698 4698 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/bookings/filter/BookingFilterListScreen.kt
Adds a new `BookingTypeFilterViewModel` to manage the state for the booking type filter. This ViewModel handles the initial selected type and updates the UI state when a new type is selected.
JorgeMucientes
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.
Nicely done @irfano. Code looks good.
I just left a minor concern that might worth checking before merging this.
Additionally, I was wondering if within the UI scope work we are doing here, shouldn't this PR also include the logic to update BookingFilterListScreen with the selected value from BookingTypeFilterPage? It feels like that part would make sense as part of the UI work too.
| TODO() | ||
| BookingTypeFilterRoute(initialType = state.currentBookingType) { type -> | ||
| state.onUpdateFilterOption(type) | ||
| } |
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'm a bit confused with this new filter "BookingType" and the one added below in line 133 "Service/Event". I think we are adding the same filter twice. Might be worth double checking with Wagner or with folks from ciab-bookings how do they want to refer to this filter.
Currently in desktop experience they are referring to it as Service/Event filter. But who knows if that is the final call:
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 plan to handle that in a separate PR. The change will involve updating the |
| sealed interface BookingType : BookingsFilterOption { | ||
| object Any : BookingType | ||
| object Service : BookingType | ||
| object Event : BookingType | ||
| } |
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.
@irfano I'm late, but I think something like this would be more consistent with other filters here. Having it like this wouldn't require different filtering logic in the ViewModel to set the state.
data class BookingType(
val value: Type
) : BookingsFilterOption {
enum class Type {
ANY, SERVICE, EVENT
}
}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.
Thanks for noticing this and sharing your feedback, Adam. This was the alternative I had in mind. I would either do this or update the onUpdateFilterOption() function, I chose the latter to avoid defining a new Type class. But your suggestion might actually be better, I’ll consider it and update BookingType in my next PR.

Part of WOOMOB-1465
Description
This adds the Type Filter page to the Booking Filter screen. It currently includes only the UI part. once we have a dedicated ViewModel for the Type Filter screen, we’ll need to bind the logic to the UI.
Test Steps
Images/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.