-
Notifications
You must be signed in to change notification settings - Fork 4.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
Add title to image and video selection sheets #23083
Conversation
* Add descriptive titles to the top of the image selection bottom sheets. This applies when selecting a image block's image for the first time or when replacing the block's existing image.
* Extend bottom sheet title support to all other media-based blocks (e.g. Image, Media & Text, Gallery, Cover blocks).
Size Change: +2 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
This looks good @guarani. Nice job! I noticed that there is not a description/title when clicking the change XXX button on the toolbar for the Cover, Media&Text, and Video blocks (it works for the Image block). Was that intentional or is that something that should be addressed? |
@guarani This is looking mostly good. In addition to @mchowning’s feedback, I have just a couple of tiny suggestions:
|
When users have already added media to image or video based blocks and then choose to replace the media, the bottom sheet that appears now shows an appropriate title.
On Android, use section titles instead of sheet titles when displaying the media picker bottom sheet.
They are no longer needed because bottom sheet panel section titles are now used.
Thanks for catching that oversight @mchowning! Fixed now.
@iamthomasbishop thanks for the feedback. I've updated this as you've suggested and it works well. My only concern now is the slightly large gap between the title and the top edge of the bottom sheet (as seen in the jpg you shared with me in a comment above). This gap is already present in other parts of the Android app though, when the section title is used as the first title on bottom sheets.
Done 👍 |
@iamthomasbishop if we eventually migrate to use the new Pull-Down Menu in iOS 14 (and perhaps the Menu component on Android) that you've suggested offline, most of this feature will easily transfer across. |
@guarani Think this looks fine, although agreed, we might want to adjust the top spacing on sheets that lead with a section title (this happens because the baseline of the text label is shifted a bit towards the bottom edge in order to provide better spacing between sections). (We don't have to do this now, let's just keep in mind for the future)
Agreed, it should translate well. |
This is a very minor nitpick, and I don't think it should block this PR at all (I'm not even sure the best way to improve it), but one thing that I noticed going back through this is that the language now reads just a little bit oddly. For example, the title "Choose image or video from", has as the first option "Choose from device" so it repeats the word "Choose". Then the next option is "Take a Photo", which doesn't flow from the title "Choose image or video from...Take a Photo". In fact, the only option that really flows naturally from the title is "Choose image or video from...WordPress Media Library." I wonder if this is worse/better in other languages. Again, this is a very minor point, and I'm commenting on it just as something for us to keep in mind going forward (cc: @iamthomasbishop ), and not because it is something I think we need to urgently fix. |
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.
LGTM! Nice work! 👍
@mchowning I agree the wording sounds a bit off. It caught my attention too while testing this PR. I wonder if we should address this now or leave it for later. These titles themselves are new, so tweaking them might be easier done now before they are translated into other languages. WDYT @iamthomasbishop? |
@guarani mchowning it's slightly awkward, but it doesn't bother me that much tbh. Perhaps removing the word "from" would help ( |
I like that idea! That feels much less awkward to me. |
Remove the word 'from' from the bottom sheet titles that are used when selecting media inside the editor
Description
Fixes wordpress-mobile/gutenberg-mobile#986
Related wordpress-mobile/gutenberg-mobile#2370
Add descriptive titles to the top of media (image and/or video) selection bottom sheets. This applies when selecting a block's content for the first time, or when replacing existing content. Affected blocks are: Image, Video, Cover, Media & Text, and Gallery.
@iamthomasbishop the ask here was to add the title to just the image block, but I think the approach here which also handles video based blocks is better for UX (due to consistency) and was easier to implement.
There are two variations for each title, Choose and Replace:
How has this been tested?
Download the iOS build and Android build to verify the bottom sheet titles for blocks that support media.
Test cases (click to expand)
Image block
Video block
Cover block
Media & Text block
Gallery block
(The Gallery block has no mechanism for replacing images, only for removing and adding them.)
Screenshots
Screenshots showing how the image picker bottom sheet looks with these changes (click to expand):
Types of changes
Checklist:
Edits