[RNMobile] Add 'Insert from URL' option to Image block

[derekblank]

What?

Fixes wordpress-mobile/gutenberg-mobile#3190 by adding "Insert from URL" option to the Image block.

Related: gutenberg-mobile#4779

Why?

"Insert from URL" is an option for the Audio, Video, and Image blocks on the web, and also for the Audio block on Gutenberg Mobile. This PR adds the same web behavior to the Image block.

How?

• Adds an onSelectURL handler to the native image block based on the equivalent code on from the web Image block.
• Adds error message handler for invalid URLs inspired by the same UI as the native File block.
• Adds MEDIA_TYPE_IMAGE to the list of source options available when selecting an Image block, which is controlled by the Media Upload component.
• Updates the Media Upload test cases to be more explicit about the media upload options for each block. (More info about test strategies and other considerations forthcoming below.)

Testing Instructions

1. Open a Post or Page
2. Insert an Image Block
3. Tap on Insert from URL in the menu options
4. Paste a valid (or invalid) URL into the text input tap Apply
5. Image should display in UI (or show error message if the URL is invalid)

20220429_1255262.mov

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @derekblank! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[derekblank]

Other considerations regarding tests that can be discussed here, or broken out for future PRs:

1. The tests have been refactored to be more clear about the media source options for each media block. With this PR change, each media option has been added to the list of expected options for each media block. The media capture options test case was moved into its own describe block so that the test output was more explicit about which block failed. Generally, I prefer to avoid nested describe blocks, but in this case it might make sense as the media blocks are a "suite" within a suite. I'd be interested in other opinions here.

    Media capture options for different media block types
      ✓ shows the correct media capture options for the Image block (116 ms)
      ✓ shows the correct media capture options for the Video block (16 ms)
      ✓ shows the correct media capture options for the Audio block (7 ms)

2. I am still searching for an elegant way to test unhappy test paths, as in testing when a media source option should NOT be in the menu. I had experimented with passing the expected number of options that should be in each media block menu (e.g., something like expect(items.length).toBe(4)), but that didn't quite feel right.
3. The media source options appear to be "dynamic" for different blocks, depending on what context they are loaded from. For example, the Audio block appears to have different menu options based on whether it is being used on a free or Pro site plan. When the site is free, only the Insert from URL option is present. When the site is "Pro", the options are WordPress Media Library, Insert from URL, and Choose from device. This requires a bit of further investigation on free and paid plans to note exactly which circumstances the correct media source options should appear, and if this dynamic free/pro behavior occurs on any other media blocks.
4. Similar to the above, the Image block has two extra options when it is called from the native app that are not present in the Image block for the Gutenberg Mobile Demo app. (These two options are Choose from Free Photo Library and Choose from Tenor.) These come from getOtherMediaOptions within the native apps. Further investigation is required to understand how including these two options could (and/or should) be incorporated into the testing strategy.

[hypest]

Thanks for opening this draft PR @derekblank , and for putting good focus on the (automated) test strategy as well!

May I suggest splitting the effort to 2 different PRs, one to hold the main functionality plus some basic tests, and a second PR to host the extra tests? That way, the main feature moves through the review "pipeline" and becomes available sooner. WDYT?

[mchowning]

Thanks for this @derekblank ! This is looking good.

In testing this though, when I select a URL to insert into an image or video block, I'm not seeing the block updated with the selected URL (i.e., the block placeholder remains and the block's HTML markup doesn't include the URL). Is this working for you?

Generally, I prefer to avoid nested describe blocks, but in this case it might make sense as the media blocks are a "suite" within a suite. I'd be interested in other opinions here.

I like the nesting here myself. 🙂

I am still searching for an elegant way to test unhappy test paths, as in testing when a media source option should NOT be in the menu. I had experimented with passing the expected number of options that should be in each media block menu (e.g., something like expect(items.length).toBe(4)), but that didn't quite feel right.

This seems ok to me. If we're verifying that each item we expect to be there is there, and then we also check that there are not more items than we expect, then that would catch if there were any unexpected values.

Obviously if we could find a way to more directly test that exactly the elements we expect and no others are present, then that would be better, but if there isn't a straightforward way to do that, then this doesn't feel like an unreasonable approach.

The media source options appear to be "dynamic" for different blocks, depending on what context they are loaded from.... . This requires a bit of further investigation on free and paid plans to note exactly which circumstances the correct media source options should appear, and if this dynamic free/pro behavior occurs on any other media blocks.

It looks like with the current implementation the capability to upload audio and __experimentalOnlyMediaLibrary are the only two dynamic influences on the media options for Gutenberg mobile. I think capturing that behavior is all you need to worry about. Making the options more dynamic than they are now is something I'm already working on in a separate PR, so I don't think you need to handle that.

hypest: May I suggest splitting the effort to 2 different PRs, one to hold the main functionality plus some basic tests, and a second PR to host the extra tests?

I think the modifications you have to the tests in this PR already are fine to keep in this PR. If/when you look to do some of the more extensive refactorings you mentioned (accounting for unhappy paths, dynamic block behavior, or media options sent from native), I agree with @hypest that a separate PR may be a better home for those changes. Based on our earlier conversations, I think that is what you were already planning to do, so I'm more just saying this "out loud". 🙂

[mchowning]

It looks like there may be some issues with the getByText -> findByText change on CI. Here's the output in case you can't access it.

CI test failures

Summary of all failing tests
FAIL packages/format-library/src/link/test/index.native.js
  ● Android › improves apply animation performance by dismissing keyboard beforehand

    Unable to find node on an unmounted component.

      36 | 	} );
      37 |
    > 38 | 	describe( 'Media capture options for different media block types', () => {
         | 	             ^
      39 | 		const expectOptionForMediaType = ( mediaType, expectedOption ) => {
      40 | 			const wrapper = render(
      41 | 				<MediaUpload

      at Object.findFn [as findByText] (node_modules/@testing-library/react-native/build/helpers/makeQueries.js:95:35)
      at expectOptionForMediaType (packages/block-editor/src/components/media-upload/test/index.native.js:38:15)
      at Object.<anonymous> (packages/block-editor/src/components/media-upload/test/index.native.js:42:7)

  ● Android › improves apply animation performance by dismissing keyboard beforehand

    Unable to find node on an unmounted component.

      36 | 	} );
      37 |
    > 38 | 	describe( 'Media capture options for different media block types', () => {
         | 	             ^
      39 | 		const expectOptionForMediaType = ( mediaType, expectedOption ) => {
      40 | 			const wrapper = render(
      41 | 				<MediaUpload

      at Object.findFn [as findByText] (node_modules/@testing-library/react-native/build/helpers/makeQueries.js:95:35)
      at expectOptionForMediaType (packages/block-editor/src/components/media-upload/test/index.native.js:38:15)
      at Object.<anonymous> (packages/block-editor/src/components/media-upload/test/index.native.js:45:7)

  ● iOS › improves back animation performance by dismissing keyboard beforehand

    Unable to find node on an unmounted component.

      36 | 	} );
      37 |
    > 38 | 	describe( 'Media capture options for different media block types', () => {
         | 	             ^
      39 | 		const expectOptionForMediaType = ( mediaType, expectedOption ) => {
      40 | 			const wrapper = render(
      41 | 				<MediaUpload

      at Object.findFn [as findByText] (node_modules/@testing-library/react-native/build/helpers/makeQueries.js:95:35)
      at expectOptionForMediaType (packages/block-editor/src/components/media-upload/test/index.native.js:38:15)
      at Object.<anonymous> (packages/block-editor/src/components/media-upload/test/index.native.js:48:7)


Test Suites: 1 failed, 57 passed, 58 total
Tests:       2 failed, 3 skipped, 708 passed, 713 total
Snapshots:   84 passed, 84 total

[mchowning]

Adding @fluiddot as a second reviewer here since I'll be AFK for most of next week. Also, Carlos will probably be very helpful with the test improvements you're considering.

[mchowning]

What do you think about also getting up a gutenberg-mobile PR pointing at these changes so we can make sure the CI there is happy?

[fluiddot]

Hey @derekblank, following this comment, I focused on checking the unit tests and found out some parts that might be related to the failures. Let me know if you need help addressing them, thanks 🙇 .

On a different note, I see that the PR is a draft (i.e. not ready to review), so I wanted to check with you if I should proceed reviewing it or wait for further updates.

[fluiddot]

I think part of the failures in the unit tests is related to this line, findByText function returns a Promise so we should update expectOptionForMediaType function to be asynchronous, as well as the test cases where it's used.

[derekblank]

I updated expectOptionForMediaType to be asynchronous. Making the other test cases where it is used asynchronous triggered the linter warning rule for jest/expect-expect, which surprised me. The tests pass without using async on these other test cases where expectOptionForMediaType is being called, so I'm not certain if something unintended is happening here.

[fluiddot]

The name of the function references the word expect, however, I don't see any expect statement within the function. I think we could check if the node returned by wrapper.findByText( expectedOption ) is visible doing something like:

const option = await wrapper.findByText( expectedOption );
expect( option ).toBeVisible();

[fluiddot]

As an additional note, I noticed that the second argument's name expectedOption is singular but in the new test cases added (reference), we're passing an array. This might be also producing failures in the unit tests as the ByText query expects either a string or a regular expression, not an array.

[derekblank]

I think part of the failures in the unit tests is related to this line, findByText function returns a Promise so we should update expectOptionForMediaType function to be asynchronous, as well as the test cases where it's used.

Good callout on making expectOptionForMediaType asynchronous, and also adding an expect statement within the function too. I've added both of these changes.

As an additional note, I noticed that the second argument's name expectedOption is singular but in the new test cases added (reference), we're passing an array. This might be also producing failures in the unit tests as the ByText query expects either a string or a regular expression, not an array.

I think this is a good callout as well. To be honest, I wasn't sure if passing the text strings as an array to expectOptionForMediaType was really improving the tests' code readability, or if it was just making things more complicated. (This is also why the PR is still a draft as I wanted to get some feedback on this approach, so thank you. 🙇 ) After thinking about it a bit more, I think keeping getByText as the query is better, so I used forEach to iterate over each option in the array:

await expectedOptions.forEach( ( item ) => {
    const option = wrapper.getByText( item );
    expect( option ).toBeVisible();
} );

What do you think about this approach @fluiddot ?

[fluiddot]

think this is a good callout as well. To be honest, I wasn't sure if passing the text strings as an array to expectOptionForMediaType was really improving the tests' code readability, or if it was just making things more complicated. (This is also why the PR is still a draft as I wanted to get some feedback on this approach, so thank you. 🙇 )

To be honest, I don't have a strong opinion on whether having that function accepts one option or multiple 🤔 .

Regarding this function, my gut feeling is that the logic could be eventually moved to a generic helper, as it's likely to be used on other test cases. So, in that case, I'd advocate having the one option only so we keep the helper simple, and then let the test cases handle the multiple options case. I think it might even help with the readability of the tests.

After thinking about it a bit more, I think keeping getByText as the query is better, so I used forEach to iterate over each option in the array:

await expectedOptions.forEach( ( item ) => {
    const option = wrapper.getByText( item );
    expect( option ).toBeVisible();
} );

What do you think about this approach @fluiddot ?

I think that's a good approach 👍. The only thing I'd like to mention is that if we use getByText, since it's a synchronous function, we wouldn't need to use await, and probably make expectOptionForMediaType asynchronous.

[derekblank]

Thanks for the feedback @mchowning !

In testing this though, when I select a URL to insert into an image or video block, I'm not seeing the block updated with the selected URL (i.e., the block placeholder remains and the block's HTML markup doesn't include the URL). Is this working for you?

No, it is not working for me either on the image and video blocks. I had attributed this to the differences between native and the Gutenberg Mobile demo app, and hadn't yet tested the Audio block (where Insert from URL it is working). Looking at the Audio block code, it looks the Image and Video blocks will need similar code to trigger the onSelect. I had a feeling that adding the Insert from URL option to the Media Upload options was a deceptively simple code change. 😅 So, that should be the next step for this task and PR. I also made a note to discuss HTML markup for blocks more in-depth during our next 1:1.

It looks like with the current implementation the capability to upload audio and __experimentalOnlyMediaLibrary are the only two dynamic influences on the media options for Gutenberg mobile. I think capturing that behavior is all you need to worry about. Making the options more dynamic than they are now is something I'm already working on in #39769, so I don't think you need to handle that.

Sounds good! I'll definitely check out those tests in your PR as well. 👍

[derekblank]

@mchowning A couple of things to discuss:

• I've updated the Image and Video files to receive the correct file from MediaUpload. Both blocks are now receiving the correct file back to the component. The Image block is working great for me to show the new image and update the block's HTML. The Video block is showing the correct file change in the Javascript, but the change is not reflected in the UI when using the Gutenberg Mobile Demo app. Simply, nothing happens. This behavior is the same for me for all of the other Media Select options for the Video block (Take a Video, Upload from Device, Wordpress Media Library). I had only tested with the Gutenberg Mobile Demo app, so perhaps this is expected and requires testing in one of the native apps. If so, we can discuss best practices for testing local Gutenberg changes in the native apps, which I have a few questions about anyways.
• When an invalid URL is given to the Image and Video blocks, I was trying to match the error state precedent given by the Audio block, which uses createErrorNotice (and useDispatch) to send an error message: "Invalid URL. Audio file not found". However, the Audio block is a functional component and the Image and Video blocks are class components, so the useDispatch hook can't be used for Image and Video without refactoring them to be functional components. That's an option perhaps for a later PR, but I was searching for a convention that could send a similar error notice from a class component using withDispatch. (Reference for where this might occur in the Image component from this PR.) Is this UI pattern for sending error notices still used in class based components? Another alternative would be to consider that perhaps this is not a significant use case and just let the URL handling fail silently if it is not a valid URL. Open to other ideas I haven't considered yet as well. (@fluiddot may have encountered this before, too.) Here's a visual reference from the Audio block:
  

[mchowning]

Should we be setting src instead of url here?

[mchowning]

The web implementation first checks if the url can be converted into an embed. I think we'll need to do something similar in order to handle things like Youtube links well?

[mchowning]

👋 Derek! Nice progress on this. 😄

The Video block is showing the correct file change in the Javascript, but the change is not reflected in the UI when using the Gutenberg Mobile Demo app. Simply, nothing happens.

I think this is because the render method returns early with the placeholder if the id attribute is undefined, and we're not setting an id when inserting a URL (because we don't have one). Might be worth checking the web implementation to see how they handle this.

This behavior is the same for me for all of the other Media Select options for the Video block (Take a Video, Upload from Device, Wordpress Media Library). I had only tested with the Gutenberg Mobile Demo app, so perhaps this is expected and requires testing in one of the native apps.

You've got good instincts here. 🙂 Those other media options are pretty closely tied to native, and for anything that is closely tied to native it's a good idea to test it using the WordPress apps because the demo app doesn't support a lot of them (and I believe the Android demo app actually supports even fewer of them than the iOS demo app). FWIW though, I think we probably can get the "Insert from URL" functionality working in the demo app.

the useDispatch hook can't be used for Image and Video without refactoring them to be functional components. That's an option perhaps for a later PR...

👍 That would be a great improvement but I agree, let's not worry about it in this PR.

...so I was searching for a convention that could send a similar error notice from a class component using withDispatch.

It looks like the file block is still a class-based component and has some error handling. Do you think we can use anything from that here?

Another alternative would be to consider that perhaps this is not a significant use case and just let the URL handling fail silently if it is not a valid URL.

I think we need to do something to inform the user what has gone wrong, otherwise it would be pretty frustrating if you thought you had a valid URL.

The video block side of this is obviously turning out to be a fair bit more complicated. I don't feel strongly either way at this point, but if it continues being difficult, it might be worth pulling the Video block changes out and putting them into a separate PR.

[fluiddot]

@derekblank I tested the PR again and is working nice 🎊 . However, before approving it, it would be great if we could review the comment regarding setting the width and height of the image I added. Let me know if you'd like to expand information about it, thanks 🙇 .

[derekblank]

Thanks for the review @fluiddot! I updated and responded to the final comment regarding the image dimensions for the web. With that change done, I've re-tested all of the flows for web and native using "Insert from URL" on the Image block, and did not note any other issues. If your testing experience is the same, I think we could say that any further issues should be handled via another PR. 🚀

I'll be creating a "retro" post of sorts to add to my onboarding P2 reflecting on what went well with this PR and what I can improve for next time. Feel free to consider anything you might have to add and include it here, or on the post when it is created. As always, your review and feedback is much appreciated! 🙇

[fluiddot]

Thanks for the review @fluiddot! I updated and responded to the final comment regarding the image dimensions for the web. With that change done, I've re-tested all of the flows for web and native using "Insert from URL" on the Image block, and did not note any other issues. If your testing experience is the same, I think we could say that any further issues should be handled via another PR. 🚀

@derekblank thanks for addressing all the issues 🙇 , I re-tested the changes and confirm that I haven't found further issues, so the PR is ready to be merged 🎊 !

As an additional note, it might be interesting if in the future we also add the Insert from URL option to the Gallery block. I noticed that with these changes, it allows to insert an image from an URL for already added items but not for new ones.

ios-gallery-block-insert-url.mp4

[fluiddot]

I've just realized that we didn't receive feedback from @iamthomasbishop regarding the loading indicator, Thomas, could you take a quick look when you're available? Thanks 🙇 !

Sharing here a couple of screenshots to showcase how the loading indicator looks with these changes:

Empty Image block	Image block with an image

Note that this loading indicator is only shown after inserting the URL of an image, while the editor checks if the image exists (this behavior can also be checked in the video capture of this comment).

[iamthomasbishop]

@fluiddot @derekblank Thanks for the pings! The loading indicators look okay as-is, although I'd like to propose some refinements:

• The iOS one in particular looks a bit weak in terms of contrast. Do we have control over the color/style of the loading indicators? If so, I'd suggest that we make the base color white on iOS. For Android, I would suggest that we change the base color from the standard green-ish to our primary accent blue (blue 50 in light, blue 30 in dark). Note: my suggestion assumes that the scrim/overlay is always dark — if we're inverting the overlay between appearance modes, we'd also invert the colors mentioned above)
• The indicators look a bit small (especially on Android) so I think we might want to increase the size of both slightly (by either 1.5x or 2x), but let's try implementing the color details I mentioned above first to see if that is sufficient. I haven't tried this on a device yet so it might be less of a concern on-device.

[derekblank]

As an additional note, it might be interesting if in the future we also add the Insert from URL option to the Gallery block. I noticed that with these changes, it allows to insert an image from an URL for already added items but not for new ones.

@fluiddot I agree - I've added issues to update blocks that use nested Image blocks, starting with the Gallery block, but also including the Cover/Media & Text blocks:

• Add Insert from URL option to Gallery block
• Add Insert from URL to Cover and Media & Text block images

[derekblank]

@fluiddot @derekblank Thanks for the pings! The loading indicators look okay as-is, although I'd like to propose some refinements

Thanks for the feedback @iamthomasbishop! I agree that your style suggestions would improve the visibility of the loading indicators. This loading indicator pattern could potentially be used (and standardized) across a handful of other blocks that fetch media using network requests, too. Rather than implement ad hoc style updates for the Image block alone here, I think it would be good to move the broader discussion to an issue that considers the loading indicators to be a shared style that can be reused across these other blocks, so I've created that issue here: Use shared loading indicator styles. I've added your comment from above to that issue to kick off the design discussion. Let me know if you have any further thoughts, of course.

[fluiddot]

@derekblank Since you don't have permission to merge this PR and all PR checks are 🟢 , I'll go ahead and merge it 🎊 .

[iamthomasbishop]

Rather than implement ad hoc style updates for the Image block alone here, I think it would be good to move the broader discussion to an issue that considers the loading indicators to be a shared style that can be reused across these other blocks, so I've created that issue here

@derekblank sounds good, and thank you!