Add isAppender functionality on mobile

[lukewalczak]

Description

This PR is adding isAppender functionality to mobile MediaPlaceholder component.

Related gutenberg-mobile PR

See wordpress-mobile/gutenberg-mobile#1299

According to the design, isAppender button should be displayed below the gallery once an item in the gallery is selected, that's why information, whether a block is selected, has to be passed from parent component into media-placeholder component.

Web implementation was not touched.

How has this been tested?

Tested manually in local gutenberg using gallery, however, currently it's hard to test it, so I have to do some tweaks in image component to treat it as gallery since the gallery component is not prepared for mobile yet.

Generally, my goal was to adjust image component and do some code changes within image/ edit.native.js to behave similarly to gallery component. To accomplish that I've changed:

• added isAppender prop and make it conditional on hasImage value
• added isSelected prop
• if there is not image return mediaPlaceholder like in gallery

		const hasImage = !! url;

		const mediaPlaceholder = (
				<MediaPlaceholder
					allowedTypes={ [MEDIA_TYPE_IMAGE] }
					onSelect={ this.onSelectMediaUploadOption }
					icon={ this.getIcon( false ) }
					onFocus={ this.props.onFocus }
					isAppender={ hasImage }
					disableMediaButtons={ !isSelected }
				/>
		)

		if ( ! hasImage ) {
			return mediaPlaceholder;
		}

then image component is returning a chosen photo along with mediaPlacholder the same as in gallery

		return (
			<MediaUpload
				allowedTypes={ [MEDIA_TYPE_IMAGE ]}
				onSelect={ this.onSelectMediaUploadOption }
				render={ ( { open, getMediaOptions } ) => {
					return (
						<>
							{getImageComponent( open, getMediaOptions )}
							{mediaPlaceholder}
						</>
					)
				} }
			/>
		);

Screenshots

Types of changes

• Support for isAppender prop on mobile platform
• New prop isSelected for mobile to distinguish if block is selected

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.

[pinarol]

Could you please update the target branch as rnmobile/master on this PR as well? We have a blocker issue that prevents us moving fwd with the master branch.

[pinarol]

It doesn't look like isSelected needs to be part of MediaPlaceholder API. Block can decide to render/not render the MediaPlaceholder due to its selected state.
Also as far as I see web also handles it this way without needing to add this to MediaPlaceholder API and we want to come close to web as much as possible.
Let me know if I am missing something.

[lukewalczak]

According to the mobile gallery design, isAppender button should be displayed only when gallery block is selected, that's why there is a need to pass isSelected value from parent component into mediaPlaceholder. Please, correct me if I understood it wrongly.

[pinarol]

Could you be able to do some investigation about how this works on web? As far as I see mediaplaceholder isn't rendered if block is not selected on web as well. So how is it handled without some isSelected prop on Gallery block? We want to keep props of components as similar as possible between platforms.
Independent from this, it doesn't sound right for a component to have a prop named 'isSelected' where it actually decides rendering/not rendering it. 'isSelected' belongs to blocks and not applicable here as is. We need to change it to describe what it does for MediaPlaceholder.

[lukewalczak]

Sure, will investigate it!

[lukewalczak]

After the investigation, I've noticed that web media-placeholder accepts prop called dropZoneUIOnly={ hasImages && ! isSelected } which then within placeholder is rendering gallery with appender when dropZoneUIOnly === false otherwise without appender.

[pinarol]

maybe we can make use of the same prop for parity, although, we don't currently have drag & drop of media. But it looks like a nice enhancement for the future. wdyt @koke ? I think we can start using dropZoneUIOnly on mobile too.

[koke]

I don't think dropZoneUIOnly is the right prop to use here. I checked the README and it's not documented anywhere. From what I'm seeing, as long as media upload is possible, the drop zone would be rendered on the web, and the UI to pick media is the variable here.

Adding dropZoneUIOnly to mobile would imply that we support drag and drop, and could be confusing. I'd rather flip the logic and have a showMediaSelectionUI (or another better name) prop that is set only when the block is selected.

[lukewalczak]

The name you've proposed (showMediaSelectionUI) sounds good for me. Just to be sure, would you like to replace dropZoneUIOnly with showMediaSelectionUI on the web and have the same prop on mobile?

[koke]

Yes, I would propose that change for the web as well

[pinarol]

LGTM as soon as we keep the parity. 👍

[pinarol]

@gziolo 👋 would changing the prop name here be a problem? Are there some steps we need to take before changing it?

[gziolo]

It'd be a breaking change as this prop is part of the public API.

The corresponding PR was merged in March (#12367) and WordPress 5.2 went out on May 7th (https://wordpress.org/news/2019/05/jaco/). I don't see it in wp/5.2 branch so it means it was merged during the beta phase and not included in the release cycle.

Anyway, it's been a few months since it was exposed as part of the public API so we would have to keep the old name as well to ensure we don't break any plugins which might be using it.

[gziolo]

I see it documented in the master branch but not here:

https://github.com/WordPress/gutenberg/tree/master/packages/block-editor/src/components/media-placeholder#dropzoneuionly

If true, only the Drop Zone will be rendered. No UI controls to upload the media will be shown. The disableDropZone prop still takes precedence over dropZoneUIOnly – specifying both as true will result in nothing to be rendered.

@jorgefilipecosta - can you help here? As far as I can tell, this is something which on the web shows only a box where you can drop an image from your OS.

[lukewalczak]

So, as far as I understand we are not able to change that prop name.
Should we then share the dropZoneUIOnly prop (which sounds weird a bit on mobile) between platforms?

[pinarol]

When I look at the latest docs I see that disableDropZone and dropZoneUIOnly are also related: The disableDropZone prop still takes precedence over dropZoneUIOnly – specifying both as true will result in nothing to be rendered Thus, just renaming it as showMediaSelectionUI won't make sense unless we refactor it to work independently.
AFAIU, we'll either need to change the behavior of web implementation too or we'll keep the props same but we'll need to add to docs that drag & drop is not yet supported on mobile. I am aiming towards to the latter one since that's a nice feature to have on mobile too(in the future). wdyt @koke ?

[koke]

That makes sense. For consistency with the other prop I would suggest disableMediaSelection instead of “hide”

[pinarol]

disableMediaSelection sounds good to me 👍

[pinarol]

@jorgefilipecosta let us know if you have concerns about this. Otherwise I think we can go ahead and deprecate dropZoneUIOnly and add disableMediaSelection?

[lukewalczak]

@pinarol I've updated, my PR according to the conversation above:

• added disableMediaSelection prop
• deprecatedropZoneUIOnly prop

[lukewalczak]

One test is failing block-transforms because of deprecated warning. I will be grateful if someone can look at it and help me with that.

[pinarol]

Looks good to me! 🎉

Tested both with iOS example app and local gutenberg web instance. Although I think we should wait for @jorgefilipecosta 's approval to merge.

[jorgefilipecosta]

Tested the web version and it works well, the PR seems ready.
There is only one step we should do before the merge, add a "Breaking Change" changelog entry in packages/block-editor/CHANGELOG.md specifying the dropZoneUIOnly prop was deprecated and disableMediaButtons should now be used instead.

[gziolo]

add a "Breaking Change" changelog entry

@jorgefilipecosta It's only a deprecation for the time being which triggers minor version updated. Only when functionality is completely removed, it's considered a breaking change. Regardless, thank you for ensuring we have an entry included in the changelog :)

[gziolo]

@lukewalczak, great work and congrats on your first code contribution to Gutenberg 🎉