-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Updater picker empty list message to match media type #16050
Conversation
Generated by 🚫 dangerJS |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APKs: |
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.
Just a few small issues, but nothing major, so I'll go ahead and give a 👍 because this worked great in my testing. Thanks Jason! 🙇
I did notice that this is a slightly different approach to how this is handled in the MediaGallery Fragment, which uses MediaFilters. I think that's fine because I like how simple this implementation is, but I wanted to at least call it out since it's always nice to use the same approach when possible.
WordPress/src/main/java/org/wordpress/android/ui/mediapicker/MediaPickerViewModel.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/mediapicker/loader/MediaLibraryDataSource.kt
Outdated
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/ui/mediapicker/MediaPickerViewModel.kt
Outdated
Show resolved
Hide resolved
@@ -152,6 +154,7 @@ data class MediaLoader( | |||
|
|||
data class DomainModel( | |||
val domainItems: List<MediaItem> = listOf(), | |||
val mediaTypes: Set<MediaType>? = null, |
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.
Unless we want a null
value for mediaTypes
to be distinguishable/treated-differently from from an "empty set" of mediaTypes
, I would be inclined to not allow this parameter to be null (i.e., to change it from Set<MediaType>?
to Set<MediaType>
) and to instead use emptySet()
anywhere it would be null
.
I just like to disallow null
values if it makes sense. What you have now is fine though, and I think this falls in the realm of personal preference, so feel free to leave it as-is if you prefer.
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 probably not as nullphobic as I should be but there might be a reason for this to be null. That might however be an artifact from when I was trying to figure out the new interface to hold the mediaTypes. I'm guessing I can switch this out to use an emptySet
Plus update per feedback
Fixes wordpress-mobile/gutenberg-mobile#3167
To test:
0. Choose a site with no media in the WordPress media library
Regression Notes
Potential unintended areas of impact
This touches how the media picker collects files and could impact non empty file systems
What I did to test those areas of impact (or what existing automated tests I relied on)
I tried the picker on sites with and with out media
What automated tests I added (or what prevented me from doing so)
None were added.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.