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.

Sign up for GitHub

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

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

Merged
merged 25 commits into from
May 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e686dde
Add 'Insert from URL' option to Video and Image blocks
Apr 7, 2022
3b9afa3
Merge branch 'WordPress:trunk' into rnmobile/add-url-option-to-video-…
derekblank Apr 7, 2022
cec7316
Update code style from linting
derekblank Apr 11, 2022
0b48a73
Merge branch 'rnmobile/add-url-option-to-video-and-image-blocks' of h…
derekblank Apr 11, 2022
d56ab56
Improve test cases for Media Upload capture options
derekblank Apr 14, 2022
504eabf
Fix whitespace issue
derekblank Apr 14, 2022
d5394d0
Merge branch 'WordPress:trunk' into rnmobile/add-url-option-to-video-…
derekblank Apr 14, 2022
8b79cf1
Update Media Upload option tests to be asynchronous
derekblank Apr 21, 2022
c660f75
Merge branch 'rnmobile/add-url-option-to-video-and-image-blocks' of h…
derekblank Apr 21, 2022
760b9cf
Update native image block to use correct image URL
derekblank Apr 26, 2022
b3312b9
Add error handling for invalid URLs to native Image block
derekblank Apr 28, 2022
badb6e1
Clear invalid URL error on Image URL success
derekblank Apr 29, 2022
ee5456b
Fix synchronicity of Media Upload option tests
derekblank Apr 29, 2022
0e647c1
Add check for URL handler to native Image block picker options
derekblank May 5, 2022
4d33bb1
Update code style
derekblank May 5, 2022
aede9d3
Remove Video block from urlSource options
derekblank May 5, 2022
dbc6e23
Remove URL option from Video block for Media Upload test
derekblank May 5, 2022
07ee97d
Use Notice snackbar for native Image block error handling
derekblank May 6, 2022
7e8b6cb
Update Image/Media Upload code style and helpers
derekblank May 9, 2022
7995ef9
Use getImage to determine if URL is a valid image within Image block
derekblank May 13, 2022
344c340
Add loading indicator and isURL check to native Image block URL behavior
derekblank May 16, 2022
ee61ccb
Add loading indicator to native Image block media placeholder
derekblank May 18, 2022
a43fce1
Fix whitespace issue in native Image block code style
derekblank May 19, 2022
7b721c0
Reuse native Image block loading indicator
derekblank May 20, 2022
ae02763
Use undefined dimension attributes for the native Image block URL beh…
derekblank May 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/block-editor/src/components/media-upload/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ export const MEDIA_TYPE_ANY = 'any';

export const OPTION_TAKE_VIDEO = __( 'Take a Video' );
export const OPTION_TAKE_PHOTO = __( 'Take a Photo' );
export const OPTION_CHOOSE_FROM_DEVICE = __( 'Choose from Device' );
Copy link
Contributor

Choose a reason for hiding this comment

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

I realized that this constant is not used anywhere although is exported. Not a big issue but I was wondering if it was added for a different purpose 🤔 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially I had added it for a test case that ended up not being used, so I agree that it could be removed. 👍

export const OPTION_TAKE_PHOTO_OR_VIDEO = __( 'Take a Photo or Video' );
export const OPTION_INSERT_FROM_URL = __( 'Insert from URL' );
export const OPTION_WORDPRESS_MEDIA_LIBRARY = __( 'WordPress Media Library' );

const URL_MEDIA_SOURCE = 'URL';

Expand Down Expand Up @@ -124,7 +126,7 @@ export class MediaUpload extends Component {
id: URL_MEDIA_SOURCE,
value: URL_MEDIA_SOURCE,
label: __( 'Insert from URL' ),
types: [ MEDIA_TYPE_AUDIO ],
types: [ MEDIA_TYPE_AUDIO, MEDIA_TYPE_IMAGE, MEDIA_TYPE_VIDEO ],
icon: globe,
};

Expand Down
34 changes: 29 additions & 5 deletions packages/block-editor/src/components/media-upload/test/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@ import {
MEDIA_TYPE_IMAGE,
MEDIA_TYPE_VIDEO,
MEDIA_TYPE_AUDIO,
OPTION_CHOOSE_FROM_DEVICE,
OPTION_TAKE_VIDEO,
OPTION_TAKE_PHOTO,
OPTION_INSERT_FROM_URL,
OPTION_WORDPRESS_MEDIA_LIBRARY,
} from '../index';

const MEDIA_URL = 'http://host.media.type';
Expand All @@ -33,7 +35,7 @@ describe( 'MediaUpload component', () => {
expect( wrapper ).toBeTruthy();
} );

it( 'shows right media capture option for media type', () => {
describe( 'Media capture options for different media block types', () => {
const expectOptionForMediaType = ( mediaType, expectedOption ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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();

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

@fluiddot fluiddot Apr 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

const wrapper = render(
<MediaUpload
Expand All @@ -52,11 +54,33 @@ describe( 'MediaUpload component', () => {
);
fireEvent.press( wrapper.getByText( 'Open Picker' ) );

wrapper.getByText( expectedOption );
wrapper.findByText( expectedOption );
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

};
expectOptionForMediaType( MEDIA_TYPE_IMAGE, OPTION_TAKE_PHOTO );
expectOptionForMediaType( MEDIA_TYPE_VIDEO, OPTION_TAKE_VIDEO );
expectOptionForMediaType( MEDIA_TYPE_AUDIO, OPTION_INSERT_FROM_URL );

it( 'shows the correct media capture options for the Image block', () => {
expectOptionForMediaType( MEDIA_TYPE_IMAGE, [
OPTION_CHOOSE_FROM_DEVICE,
OPTION_TAKE_PHOTO,
OPTION_WORDPRESS_MEDIA_LIBRARY,
OPTION_INSERT_FROM_URL,
] );
} );

it( 'shows the correct media capture options for the Video block', () => {
expectOptionForMediaType( MEDIA_TYPE_VIDEO, [
OPTION_CHOOSE_FROM_DEVICE,
OPTION_TAKE_VIDEO,
OPTION_WORDPRESS_MEDIA_LIBRARY,
OPTION_INSERT_FROM_URL,
] );
} );

it( 'shows the correct media capture options for the Audio block', () => {
expectOptionForMediaType( MEDIA_TYPE_AUDIO, [
OPTION_WORDPRESS_MEDIA_LIBRARY,
OPTION_INSERT_FROM_URL,
] );
} );
} );

const expectMediaPickerForOption = (
Expand Down