-
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
Filter for images and video in add media picker #10576
Conversation
You can test the changes on this Pull Request by downloading the APK here. |
@Override | ||
public void onAddMediaClicked(boolean allowMultipleSelection) { | ||
mAllowMultipleSelection = allowMultipleSelection; | ||
WPMediaUtils.launchMediaLibrary(this, mAllowMultipleSelection); |
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 definitely not familiar enough with Android and I'd recommend another reviewer, but I was aware of the MediaBrowserType enum, and I see that the other addMedia methods use ActivityLauncher.viewMediaPickerForResult
instead. Is there a reason not to use the same thing?
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 question! The viewMediaPickerForResult
method actually prepares an intent with the "in-house" MediaBrowserActivity
(i.e. "WordPress Media Library"). At first glance at the code, I had the same thought, and that may be an indication that the name is ambiguous. OTOH, I'm not sure I can offer a better name, since the MediaBrowserActivity
itself has menu items that call through to the device picker (which, incidentally, use the WPMediaUtils
methods).
Also, I will take your advice about adding an Android reviewer (more familiar with this area of the code base) 👍
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.
The "WordPress Media Library" flow has also been addressed in 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.
Working as expected!
LGTM 👍
…th-pickers-single-item Respect allowMultipleSelection flag for media-text block
Fixes wordpress-mobile/gutenberg-mobile#1403
Companion PRs
gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1412
Can be merged after: #10567 and #10560
To test:
Follow steps listed here: WordPress/gutenberg#17537 (comment)
and make sure videos and images can be selected from the picker after choosing "Choose from device".
Also, it should be possible to select videos and images after selecting "WordPress Media Library".
Note: When selecting via "WordPress Media Library", multiple items may be selected. This is a known issue, and the current behavior should be that a single image or video is used for the media-text block, while the rest are inserted below as image or video blocks. This issue is addressed in this PR: #10597.
Screencasts
PR submission checklist:
I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to
RELEASE-NOTES.txt
if necessary.