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

[Mobile] Gallery - Media editing #24088

Merged
merged 13 commits into from
Aug 14, 2020
Merged

[Mobile] Gallery - Media editing #24088

merged 13 commits into from
Aug 14, 2020

Conversation

geriux
Copy link
Member

@geriux geriux commented Jul 21, 2020

Gutenberg Mobile PR -> wordpress-mobile/gutenberg-mobile#2496
WordPress iOS PR -> wordpress-mobile/WordPress-iOS#14498
WordPress Android PR -> wordpress-mobile/WordPress-Android#12497

Description

Continuing adding media editing support to blocks this PR adds it to Gallery block. There aren't any major changes other than adding the MediaUpload component in gallery-image.native to support replacing images within a gallery, other changes are wrapping some parts of the code within new containers to fix some absolute positioning issues.

It adds two new props to the Image component: height and resizeMode.

Allows disabling the Replace option in the media editing button.

Note: The replace option was removed and will be added in a future iteration.

How has this been tested?

Steps to test ---

Edit an image

  • Open the app with metro running
  • Add a Gallery block
  • Tap on the block and upload some pictures
  • Expect to see all the images being uploaded with their progress bar
  • Tap on a picture
  • Expect the image to be selected (blue border around) and the media button option to be visible at the top right
  • Tap on the media editing button
  • Expect to see the options
  • Tap on Edit
  • Expect to see the media editor
  • Edit the image and tap on done
  • Expect to see the edited picture uploaded

Replace an image

  • Open the app with metro running
  • Add a Gallery block
  • Tap on the block and upload some pictures
  • Expect to see all the images being uploaded with their progress bar
  • Tap on a picture
  • Expect the image to be selected (blue border around) and the media button option to be visible at the top right
  • Tap on the media editing button
  • Expect to see the options
  • Tap on Replace
  • Expect to see the media picker
  • Upload a new picture
  • Expect to see the new picture uploaded and replacing the previous one

Full screen preview

  • Open the app with metro running
  • Add a Gallery block
  • Tap on the block and upload some pictures
  • Expect to see all the images being uploaded with their progress bar
  • Tap on a picture
  • Expect the image to be selected (blue border around) and the media button option to be visible at the top right
  • Tap on the image again
  • Expect to see the image in full screen

Remove a picture

  • Open the app with metro running
  • Add a Gallery block
  • Tap on the block and upload some pictures
  • Expect to see all the images being uploaded with their progress bar
  • Tap on a picture
  • Expect the image to be selected (blue border around) and the media button option to be visible at the top right
  • Tap on the media editing button
  • Expect to see the options
  • Tap on Remove
  • Expect to see the selected picture removed

Screenshots

iOS Android

Types of changes

New feature

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.

@geriux geriux 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 21, 2020
@github-actions
Copy link

github-actions bot commented Jul 21, 2020

Size Change: 0 B

Total Size: 1.16 MB

ℹ️ 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.44 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.97 kB 0 B
build/block-directory/style-rtl.css 953 B 0 B
build/block-directory/style.css 952 B 0 B
build/block-editor/index.js 125 kB 0 B
build/block-editor/style-rtl.css 10.6 kB 0 B
build/block-editor/style.css 10.6 kB 0 B
build/block-library/editor-rtl.css 8.36 kB 0 B
build/block-library/editor.css 8.36 kB 0 B
build/block-library/index.js 132 kB 0 B
build/block-library/style-rtl.css 7.49 kB 0 B
build/block-library/style.css 7.49 kB 0 B
build/block-library/theme-rtl.css 729 B 0 B
build/block-library/theme.css 730 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.4 kB 0 B
build/components/index.js 200 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 9.68 kB 0 B
build/core-data/index.js 11.8 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.45 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 568 B 0 B
build/dom/index.js 3.23 kB 0 B
build/edit-navigation/index.js 10.9 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.63 kB 0 B
build/edit-post/style.css 5.63 kB 0 B
build/edit-site/index.js 17 kB 0 B
build/edit-site/style-rtl.css 3.06 kB 0 B
build/edit-site/style.css 3.06 kB 0 B
build/edit-widgets/index.js 11.7 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.3 kB 0 B
build/editor/style-rtl.css 3.8 kB 0 B
build/editor/style.css 3.79 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 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 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.33 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.41 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.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@geriux geriux force-pushed the rnmobile/gallery-media-editing branch from 61a183b to 3a6bda3 Compare July 24, 2020 09:35
@geriux geriux requested a review from mkevins July 24, 2020 09:53
@geriux geriux marked this pull request as ready for review July 24, 2020 09:53
@geriux
Copy link
Member Author

geriux commented Jul 24, 2020

Hey @iamthomasbishop 👋 now it is turn for Gallery and media editing support. Builds are ready for you to test for a design review whenever you get a chance, thank you!

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

Excited to see progress on this Gerardo!

I tested this on Android (Pixel 3a - real device) and I think the styles refactoring may have broken a few things regarding scrollable captions:

The caption is no longer scrollable, and the buttons get covered by the large caption. Also, I encountered some issues after the caption expanded beyond the size of the container, where even after deleting the text, the caption remained large, and in one case, weirdly, I couldn't tap out to unfocus the text input. I'm not sure what could cause that behavior. 🤔

Another issue I encountered was in using the replace option. Because we are in Gallery, naturally the multiple-selection flag is set, but in this case, using replace, the additional images are added as image blocks after the Gallery block. I'd expect either that the replace option only permits selection of 1 image, or that additionally selected images are inserted into the Gallery block in the position of the image they are replacing.

Replace allows multiple, but appends as Image blocks
Replace allows multiple selection Multiple images appended as Image blocks
gallery-edit-media-button-replace-allows-multiple gallery-edit-media-button-replace-multiple-append-as-image-blocks

Other than that, all the other behavior seems to be working correctly 👍

@iamthomasbishop
Copy link

@geriux I had a chance to take the new build for a spin and didn't find any big design issues. I agree w/ @mkevins feedback (nice catches, Matt!), so once those things are done, I think we're in good shape.

One thing that's semi-related but doesn't necessarily have to be part of this work -- can we utilize the mock "iOS system materials" (using the third-party library @geriux used on Compact Notices recently) on the gallery image caption background? Fine by me if we need to roll this into a separate ticket, but Matt's feedback on the caption made me think of this 😄

@geriux
Copy link
Member Author

geriux commented Aug 7, 2020

Hey @mkevins 👋

Thank you for the review and tests! Nice catches indeed 🙇‍♂️

I updated the code and made new builds with the caption fixes, I also removed the Replace functionality so we can work on a better approach for Gallery block in another iteration.

Can you please give it another review when you can? Thanks a lot!

@geriux
Copy link
Member Author

geriux commented Aug 7, 2020

Hey @iamthomasbishop 👋

One thing that's semi-related but doesn't necessarily have to be part of this work -- can we utilize the mock "iOS system materials" (using the third-party library @geriux used on Compact Notices recently) on the gallery image caption background? Fine by me if we need to roll this into a separate ticket, but Matt's feedback on the caption made me think of this 😄

That'd be a nice thing to add yes! But I think it'd be better to do it on a separate task. Is there one opened? Thanks!

@geriux geriux requested a review from mkevins August 7, 2020 08:53
@iamthomasbishop
Copy link

That'd be a nice thing to add yes! But I think it'd be better to do it on a separate task. Is there one opened? Thanks!

Most definitely can be a separate task. I will create a ticket and drop some notes on it sometime early next week.

@geriux geriux requested a review from lukewalczak August 12, 2020 07:40
@geriux
Copy link
Member Author

geriux commented Aug 12, 2020

Hey @lukewalczak 👋 I was wondering if you would be able to review this PR =)

It's a small one and most of the testing was done already, let me know! Thanks!

@geriux geriux requested a review from lukewalczak August 13, 2020 11:11
@geriux
Copy link
Member Author

geriux commented Aug 13, 2020

Hey @lukewalczak thank you for your comments! I've just updated the PR with the changes, let me know! Thanks!

Copy link
Member

@lukewalczak lukewalczak left a comment

Choose a reason for hiding this comment

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

LGTM from code perspective!

@geriux geriux requested review from mkevins and removed request for mkevins August 14, 2020 07:52
@pinarol pinarol requested review from mkevins and removed request for mkevins August 14, 2020 08:03
@pinarol
Copy link
Contributor

pinarol commented Aug 14, 2020

@mkevins is on a leave but his comments are adressed, we completely removed the replace option from this PR. @hypest could you help us merging the PR? Thanks.

@hypest
Copy link
Contributor

hypest commented Aug 14, 2020

Sure, merging using admin-rights now...

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.

6 participants