-
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
Issue/11518 media picker actions #11611
Issue/11518 media picker actions #11611
Conversation
You can test the changes on this Pull Request by downloading the APK here. |
|| this == GUTENBERG_MEDIA_PICKER; | ||
} | ||
|
||
public boolean isGutenbergPicker() { |
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.
This method is new the rest is just formatted.
# Conflicts: # WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java # WordPress/src/main/java/org/wordpress/android/util/WPMediaUtils.java
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
…18-media-picker-actions
👋 thanks for pinging for reviewing the flows @malinajirka . I haven't yet looked at all of them but the one that we actually capture a new photo in Gutenberg caught my attention: https://cloudup.com/iIP5b_K5aVO. It looks like we'll need to perhaps remove the individual "Take a photo" option in Gutenberg's media options menu? Is there a plan for that? Also, pinging @iamthomasbishop to have a look at the flows as well since those relate to the design side of things. |
We discussed this with Shakti and we thought it wouldn't hurt to have a duplicated path to achieve the same goal. I definitely wouldn't remove the individual "Take a photo" option in Gutenberg's media options menu as it's shorter. Wdyt? |
Just to clarify a bit more. We won't support editing the media before it gets uploaded in the "take a photo" and "OS default picker" paths. The media-editing will be supported only when the user selects a media in the custom picker. |
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.
Nice work @malinajirka ! I tested different media blocks, they work as described :).
Feel free to merge if flows are approved design-wise.
@malinajirka Thanks for the gifs as examples, helps a lot to visualize. A few things that stand out to me while reviewing:
|
Thanks for the reviews ;)!
We might want to consider leaving both these decisions for the Media Consolidation project (cc @frosty ). Goal of this task is to replace the OS default picker with a custom picker. This PR is re-using an existing picker which definitely could be improved, but at the same time is imo much better than the OS default picker. Unless you feel one of these issues is vital, I'd prefer to limit the scope of this task as much as possible. Wdyt @iamthomasbishop ?
This is the default OS picker and we can't change its UI. It can look differently on different devices. We want to keep it so the user can choose photos from a custom folders as our custom picker shows only content from default media folders. |
I'd prefer it if we don't have multiple paths to do the same thing in flows so close together as these, unless it's pretty much necessary. I also agree that we don't want to turn this ticket/PR into a Media Consolidation project one. So, let me ask, can we instead hide the "camera" tab of the custom picker when its client hints it only wants to choose a photo? Would that help unblock the MediaEditing project as #11518 requires? |
Yes, we could do that and it'd unblock us. @mbshakti If I recall correctly, you preferred having two buttons in the bottom bar, right? I don't recall, how would you feel about this change? |
When we made the decision to keep the camera button in the bottom bar, the reasoning was that it wouldn't hurt the user, and in case the user changes their mind, they can always use the camera. I don't think having multiple paths to do the same thing in flows is problematic, but I understand if @hypest believes so, especially since the placement is too prominent as @iamthomasbishop mentioned. We can go ahead and hide it from this view. The only weird thing there is that we would just have one button in the bottom bar then, but I don't mind that. |
@iamthomasbishop I'm sorry, I misread your comment. We could show the picker (after you select "choose from device") in a bottom sheet. However, I'd make this change in the Media Consolidation project as well - or perhaps even a different project as it's technically not related to Media Consolidation. |
Thanks @malinajirka. I tested and found camera option hidden from the custom picker in gutenberg. Merging these changes 👍. |
Fixes #11518
Final PR for the issue #11518
To test:
Gifs with all the flows https://cloudup.com/cjA9rSxls0a (cc @mbshakti could you please review if these flows look ok?)
Ideally test all the media blocks
PR submission checklist:
RELEASE-NOTES.txt
if necessary.cc @hypest @mkevins @marecar3 Could one of you please review the gifs if all the flows look as expected? Thanks! (if you want to check all the code changes compare
issue/11518-media-picker-actions
againstdevelop
- develop...issue/11518-media-picker-actions)