Skip to content
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

Merged
merged 9 commits into from
Jul 10, 2020

Conversation

guarani
Copy link
Contributor

@guarani guarani commented Jun 10, 2020

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:

  • "Choose/Replace image"
  • "Choose/Replace video"
  • "Choose/Replace image or video"

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 worked on Android
  • Test cases worked on iOS
Test cases (click to expand)

Image block

  • Add an Image block, tap the placeholder, see "Choose image"
  • With the image now added, tap the Replace toolbar button, see "Replace image"
  • Long-press the existing image, see "Replace image"
  • Tap the edit/replace button in the top-right corner of the existing image, choose Replace, see "Replace image"

Video block

  • Add a Video block, tap the placeholder, see "Choose video"
  • With the video now added, tap the Replace toolbar button, see "Choose video"

Cover block

  • Add a Cover block, tap the placeholder, see "Choose image or video"
  • With an image now added and the Cover block itself selected, tap the Replace toolbar button, see "Replace image or video"
  • Long-press the existing image, see "Replace image or video"
  • Add another Cover block, add a video, tap the Replace toolbar button, see "Replace image or video"
  • Long-press the existing video, see "Replace image or video"

Media & Text block

  • Add a Media & Text block, tap the placeholder, see "Choose image or video"
  • With an image now added and the Media & Text block itself selected, tap the Replace toolbar button, see "Replace image or video"
  • Long-press the existing image, see "Replace image or video"
  • Add another Media & Text block, add a video, tap the Replace toolbar button, see "Replace image or video"

Gallery block

(The Gallery block has no mechanism for replacing images, only for removing and adding them.)

  • Add an Gallery block, tap the placeholder, see "Choose image"

Screenshots

Screenshots showing how the image picker bottom sheet looks with these changes (click to expand):
Adding an image for the first time Replacing an existing image
iOS
Android

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

Edits

* 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).
@github-actions
Copy link

github-actions bot commented Jun 10, 2020

Size Change: +2 B (0%)

Total Size: 1.14 MB

Filename Size Change
build/block-library/index.js 132 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.67 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.67 kB 0 B
build/block-directory/style-rtl.css 944 B 0 B
build/block-directory/style.css 945 B 0 B
build/block-editor/index.js 115 kB 0 B
build/block-editor/style-rtl.css 10.8 kB 0 B
build/block-editor/style.css 10.8 kB 0 B
build/block-library/editor-rtl.css 7.6 kB 0 B
build/block-library/editor.css 7.59 kB 0 B
build/block-library/style-rtl.css 7.77 kB 0 B
build/block-library/style.css 7.77 kB 0 B
build/block-library/theme-rtl.css 728 B 0 B
build/block-library/theme.css 729 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/components/index.js 199 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.67 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.46 kB 0 B
build/date/index.js 5.38 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 1.08 kB 0 B
build/edit-navigation/style.css 1.08 kB 0 B
build/edit-post/index.js 304 kB 0 B
build/edit-post/style-rtl.css 5.59 kB 0 B
build/edit-post/style.css 5.58 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.35 kB 0 B
build/edit-widgets/style-rtl.css 2.45 kB 0 B
build/edit-widgets/style.css 2.45 kB 0 B
build/editor/editor-styles-rtl.css 537 B 0 B
build/editor/editor-styles.css 539 B 0 B
build/editor/index.js 45.1 kB 0 B
build/editor/style-rtl.css 3.78 kB 0 B
build/editor/style.css 3.78 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.32 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.4 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13.9 kB 0 B
build/server-side-render/index.js 2.71 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@mchowning
Copy link
Contributor

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?

rotate_image mp4

@iamthomasbishop
Copy link

@guarani This is looking mostly good. In addition to @mchowning’s feedback, I have just a couple of tiny suggestions:

  • On Android, is there any way we can use the standard section title instead of the larger sheet header that you’re using here? (example)

  • We can probably use sentence case on both platforms for the “choose image from” label. So the labels would become “Choose image from” and “Replace image from”.

@guarani guarani changed the title Add description/title to Image selection sheets Add description/title to Image/Video media selection sheets Jul 7, 2020
@guarani guarani changed the title Add description/title to Image/Video media selection sheets Add title to image and video selection sheets Jul 7, 2020
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.
@guarani guarani added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Jul 8, 2020
@guarani
Copy link
Contributor Author

guarani commented Jul 8, 2020

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?

Thanks for catching that oversight @mchowning! Fixed now.

On Android, is there any way we can use the standard section title instead of the larger sheet header that you’re using here? (example)

@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.
I've updated the screenshots in the PR description above to show how this feature looks, so please feel free to check those out and let me know if anything needs tweaking.

We can probably use sentence case on both platforms for the “choose image from” label. So the labels would become “Choose image from” and “Replace image from”.

Done 👍

@guarani
Copy link
Contributor Author

guarani commented Jul 8, 2020

@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.

@iamthomasbishop
Copy link

iamthomasbishop commented Jul 8, 2020

I've updated this as you've suggested and it works well

@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)

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.

Agreed, it should translate well.

@mchowning
Copy link
Contributor

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.

Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice work! 👍

@guarani
Copy link
Contributor Author

guarani commented Jul 9, 2020

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.

@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?

@iamthomasbishop
Copy link

@guarani mchowning it's slightly awkward, but it doesn't bother me that much tbh. Perhaps removing the word "from" would help (Choose [image|video]... and Replace [image|video]...)?

@mchowning
Copy link
Contributor

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
@guarani guarani merged commit c38bc99 into master Jul 10, 2020
@guarani guarani deleted the rnmobile/image-picker-title branch July 10, 2020 23:57
@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add description/title to Image selection sheets
3 participants