[RNMobile] Media & Text - Media picker buttons functionality by geriux · Pull Request #17537 · WordPress/gutenberg

geriux · 2019-09-23T23:18:26Z

Can be merged after #17392

gutenberg-mobile PR wordpress-mobile/gutenberg-mobile#1378

Description

This PR adds the missing functionality for the Media & Text block wordpress-mobile/gutenberg-mobile#1313

On Android It will only support images until we can filter by more than one type at a time (images & videos) in the media picker.

Now the functionality it's the same as the image and video blocks.

How has this been tested?

Open the WordPress app and start a new post.
Insert a new Media & Text block.
Add an image from the photo library/camera.
Expect the progress bar to show up while the file is being uploaded. While it's doing that you can cancel the upload.
Once an image is uploaded you can do a long press to open the media options to change the file.
If the connection fails, a retry option will be available.
Test the image/video can be also selected from the WordPress media library.

Screenshots

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.

koke · 2019-09-25T10:02:54Z

packages/block-library/src/media-text/media-container.native.js

+		const { mediaUrl, isSelected } = this.props;
+		const { isUploadInProgress } = this.state;
+		const { isUploadFailed, retryMessage } = params;
+		const videoHeight = ( Dimensions.get( 'window' ).width / 2 ) / VIDEO_ASPECT_RATIO;


I think you can set the aspectRatio style prop directly and avoid the calculation. Otherwise, I think using the window width would be problematic because the ratio should apply to the container width, not the full window, but I haven't been able to test this because of the other issue I commented

Confirmed that this gets the wrong aspect ratio when uploading a video on iPad:

Nice! I'll check aspectRatio and also make sure it looks great on tablets as well

koke · 2019-09-25T10:05:38Z

packages/block-library/src/media-text/media-container.native.js

+const ALLOWED_MEDIA_TYPES = [ MEDIA_TYPE_IMAGE, ...( Platform.OS === 'ios' ? [ MEDIA_TYPE_VIDEO ] : [] ) ];
+
+// Default Video ratio 16:9
+const VIDEO_ASPECT_RATIO = 16 / 9;


I don't love that we are hardcoding this, but I see we were already doing it in the video block. Can we move that logic to the newly extracted VideoPlayer component so we can improve it in the future?

Oh yes! Will do!

packages/block-library/src/media-text/media-container.native.js

geriux · 2019-09-26T08:45:28Z

Hey @koke this is ready to be reviewed again, Travis failed due to a linting issue that will be fixed in #17598

koke · 2019-09-26T15:30:55Z

I think this is looking good. I had something else in mind for the aspect ratio changes, but now I realize that the video component probably isn't as smart as images and we want a fixed ratio there. I'd probably like to have in the future an API for VideoPlayer and the placeholder component that does the right thing by itself, but I think this is good for now.

Let's try to wrap up #17392 first and merge that one and the lint fixes from master before I do the final testing round. 👍

geriux · 2019-09-27T09:12:24Z

Updated! Now all checks are green 🎉 ready to be reviewed again =)

koke · 2019-09-30T10:00:23Z

I noticed one thing that we don't have to fix in this PR but it's worth discussing with @iamthomasbishop. I wanted to change the media once I had uploaded it. When the block is selected, the toolbar is visible, but since there is no focus on any text component, the toolbar is at the bottom:

I stared at the screen and tapped on too many things for a minute trying to edit the image/video, but I just couldn't see the toolbar at the bottom.

iamthomasbishop · 2019-09-30T19:10:29Z

@koke I think the toolbar positioning is expected, but I wouldn't expect the pencil icon to be visible until I have selected the video block within the group. I know the web currently follows the above pattern, but it's always confusing to me. Shouldn't that button only be shown when I've selected the video block?

koke · 2019-10-01T09:21:55Z

@iamthomasbishop the left part of media & text doesn't use nested blocks, it's just either one image or one video, so it's part of the parent (media-text) block. I understand that was the expected design, but I still found myself confused for a bit trying to find how to replace the image/video there

iamthomasbishop · 2019-10-01T18:50:29Z

the left part of media & text doesn't use nested blocks, it's just either one image or one video, so it's part of the parent (media-text) block

Yea sorry, I thought from the example above that the parent (media-text) was selected. The replacing mechanism is definitely confusing, I've always disliked that icon. I think the web has recently updated that replace image icon (at least on the image block) to be more obvious, can we match that? Example:

The other thing that I mentioned still applies – why do we even show the image/media replace button while the parent is selected? That's confusing to me.

koke · 2019-10-04T13:54:38Z

I'll merge #17682 in a test branch to see how it look

I tried that myself when testing and it went full width but still had the same issue.

…ad for iOS and Android

geriux · 2019-10-05T14:08:52Z

@koke I've just updated this PR with the fix for the issue you mentioned, I also tested this fix with your PR and it works well too 🎉

koke · 2019-10-07T11:45:01Z

I feel like I had reported this before, but I can't find my comment anywhere so maybe I didn't? 😱

Add a media-text block
Tap Add image or video
Choose any method
Tap cancel without selecting an image
Red screen: Cannot read property 'id' of undefined

This is on iOS. I tested on Android and it seems fine.

geriux · 2019-10-07T12:57:04Z

I feel like I had reported this before, but I can't find my comment anywhere so maybe I didn't? 😱

Add a media-text block

Tap Add image or video

Choose any method

Tap cancel without selecting an image

Red screen: Cannot read property 'id' of undefined

Oh wow! I haven't seen that D: I'll check it out today. Did the other issue get fixed with the latest changes?

koke · 2019-10-07T14:23:43Z

I think this is good to go once the red screen issue is solved. 🎉

koke

I think this is ready 🎉
To avoid further issues, I'll wait until wordpress-mobile/gutenberg-mobile#1410 is merged to start merging all the related PRs

mkevins

I've tested this on Android with the additional changes here: wordpress-mobile/gutenberg-mobile#1412 and here: wordpress-mobile/gutenberg-mobile#1400. This is looking good. Nice work @geriux ! 🎉

geriux mentioned this pull request Sep 23, 2019

Media & Text - Media picker buttons functionality wordpress-mobile/gutenberg-mobile#1378

Merged

1 task

geriux marked this pull request as ready for review September 24, 2019 08:46

koke self-requested a review September 24, 2019 08:51

koke reviewed Sep 25, 2019

View reviewed changes

packages/block-library/src/media-text/media-container.native.js Outdated Show resolved Hide resolved

gziolo 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 Sep 26, 2019

geriux requested a review from koke September 26, 2019 08:45

geriux mentioned this pull request Oct 1, 2019

[Gutenberg] Return the media type for uploads wordpress-mobile/WordPress-iOS#12581

Merged

1 task

geriux force-pushed the feature/media-text-upload-progress branch from 5f32e05 to bcfd26c Compare October 2, 2019 13:22

geriux requested review from adamsilverstein, aduth, ajitbohra, atimmer, chrisvanpatten, daniloercoli, dmsnell, ellatrix, etoledom, gziolo, jorgefilipecosta, karmatosed and mapk as code owners October 2, 2019 13:22

pinarol removed request for TimothyBJacobs, adamsilverstein, aduth, ajitbohra, chrisvanpatten, dmsnell, ellatrix, jorgefilipecosta, mmtr, nerrad and ntwb October 4, 2019 11:55

Mobile - Media & Text - Fix styles and allow both types of media uplo…

7b644fe

…ad for iOS and Android

geriux requested a review from pinarol October 5, 2019 14:09

This was referenced Oct 7, 2019

Filter for images and video in add media picker wordpress-mobile/gutenberg-mobile#1412

Merged

Filter for images and video in add media picker wordpress-mobile/WordPress-Android#10576

Merged

Mobile - Media & Text - Check for existing media in callback

ace01c9

koke approved these changes Oct 8, 2019

View reviewed changes

mkevins approved these changes Oct 10, 2019

View reviewed changes

koke merged commit a5be006 into WordPress:master Oct 10, 2019

koke deleted the feature/media-text-upload-progress branch October 10, 2019 10:11

mkevins mentioned this pull request Oct 11, 2019

Respect allowMultipleSelection flag for media-text block wordpress-mobile/WordPress-Android#10597

Merged

2 tasks

youknowriad added this to the Gutenberg 6.7 milestone Oct 14, 2019

Conversation

geriux commented Sep 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

iOS

Android

Types of changes

Checklist:

Uh oh!

koke Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

koke Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

geriux Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

koke Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geriux Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geriux commented Sep 26, 2019

Uh oh!

koke commented Sep 26, 2019

Uh oh!

geriux commented Sep 27, 2019

Uh oh!

koke commented Sep 30, 2019

Uh oh!

iamthomasbishop commented Sep 30, 2019

Uh oh!

koke commented Oct 1, 2019

Uh oh!

iamthomasbishop commented Oct 1, 2019

Uh oh!

koke commented Oct 4, 2019

Uh oh!

geriux commented Oct 5, 2019

Uh oh!

koke commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

geriux commented Oct 7, 2019

Uh oh!

koke commented Oct 7, 2019

Uh oh!

koke left a comment

Choose a reason for hiding this comment

Uh oh!

mkevins left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

geriux commented Sep 23, 2019 •

edited

Loading

koke Sep 25, 2019 •

edited

Loading

koke commented Oct 7, 2019 •

edited

Loading