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

MediaUpload and MediaPlaceholder unify props #17145

Merged
merged 4 commits into from
Aug 30, 2019
Merged

MediaUpload and MediaPlaceholder unify props #17145

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
472854f
rebase to rnmobile/master
dratwas Aug 28, 2019
dc3eddd
Merge branch 'rnmobile/master' into rnmobile/media-upload-props
dratwas Aug 28, 2019
ad6c39b
fix types after merge
dratwas Aug 28, 2019
944d4e7
fix tests
dratwas Aug 28, 2019
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
16 changes: 15 additions & 1 deletion packages/block-editor/src/components/media-placeholder/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ This property is similar to the `allowedTypes` property. The difference is the f

- Type: `String`
- Required: No
- Platform: Web
Copy link
Contributor

Choose a reason for hiding this comment

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

@youknowriad 👋 Could you also be able to review README.md changes here? I believe Platform attribute is being added the first time.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this seems like a good idea to include in the README to raise awareness of the mobile side of things. I don't know if this is going to be clear enough that it's about native mobile apps. We should first provide some guidelines in the official Gutenberg docs about that and ensure that those components can work out of the box when imported as npm packages.

Can we discuss it first at the upcoming weekly Core JS meeting so people could share their opinions? See https://make.wordpress.org/meetings/.

There should also a way to include the same information in JSDoc comments as we are moving towards automated public API generation from such JSDoc comments. @nosolosw - do you think it would be easy to extend @wordpress/docgen to support it?

Copy link
Member

Choose a reason for hiding this comment

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

The biggest problem as of now is to have the components' props documented automatically. Most of them don't contain any JSDoc and, AFAIK, we haven't looked into infer anything from the internal structure (such as inferring existing props from React components).

Another thing is that there is no JSDoc mechanism to declare that a particular param is only avaliable for web or mobile. I guess we can add that to the param's description.

Copy link
Member

Choose a reason for hiding this comment

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

We discussed this topic during Core JS chat this week:

https://wordpress.slack.com/archives/C5UNMSU4R/p1566914617419700
(link requires registration at https://make.wordpress.org/chat/).

We should proceed with the platform related notes as proposed in this PR. Let's open PR which documents the process so we could replicate it for other components. It usually helps to enforce some standards when they are well documented.

@nosolosw, thanks for chiming in. We definitely will have to find a way to mark with JSDoc whether a given function or components as an entity is available on the given platform. We can start at the general level and leave props for future iterations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @gziolo ! We'll be starting such a document to describe Platform usage in README.md files, we can then iterate on it on a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

@gziolo @koke I've created #17230 to track the discussion about documentation taking into account the core-js conversation. Please, feel free to expand on what did you like about some of the examples provided (ionic, apple docs) or anything else that you find missing.

Copy link
Member

Choose a reason for hiding this comment

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

Sweet, thanks @nosolosw 👍


### addToGallery

Expand All @@ -46,6 +47,7 @@ If false the gallery media modal opens in the edit mode where the user can edit
- Type: `Boolean`
- Required: No
- Default: `false`
- Platform: Web

### allowedTypes

Expand All @@ -57,20 +59,23 @@ This property is similar to the `accept` property. The difference is the format

- Type: `Array`
- Required: No
- Platform: Web | Mobile

### className

Class name added to the placeholder.

- Type: `String`
- Required: No
- Platform: Web

### icon

Icon to display left of the title. When passed as a `String`, the icon will be resolved as a [Dashicon](https://developer.wordpress.org/resource/dashicons/). Alternatively, you can pass in a `WPComponent` such as `BlockIcon`to render instead.

- Type: `String|WPComponent`
- Required: No
- Platform: Web | Mobile

### isAppender

Expand All @@ -80,14 +85,15 @@ If false the default placeholder style is used.
- Type: `Boolean`
- Required: No
- Default: `false`
- Platform: Web

### labels

An object that can contain a `title` and `instructions` properties. These properties are passed to the placeholder component as `label` and `instructions` respectively.

- Type: `Object`
- Required: No

- Platform: Web | Mobile

### multiple

Expand All @@ -96,13 +102,15 @@ Whether to allow multiple selection of files or not.
- Type: `Boolean`
- Required: No
- Default: `false`
- Platform: Web

### onError

Callback called when an upload error happens.

- Type: `Function`
- Required: No
- Platform: Web

### onSelect

Expand All @@ -111,13 +119,19 @@ The call back receives an array with the new files. Each element of the collecti

- Type: `Function`
- Required: Yes
- Platform: Web | Mobile

The argument of the callback is an object containing the following properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the description above, the argument is an array containing those kinds of objects

Copy link
Member

@lukewalczak lukewalczak Aug 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

An array with those kinds of objects is an argument in gallery case which we are going to adjust in the second iteration. Current PR is a first iteration (simpler one). Now we are investigating possibilities of adding multiple images referring to the comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought it always returned an array on the web, but it seems the argument can be either an array or an object depending on the multiple prop. I think the existing documentation was wrong and it should explain this difference in behavior

- Web: `{ url, alt, id, link, caption, sizes, media_details }`
- Mobile: `{ id, url }`

### value

Media ID (or media IDs if multiple is true) to be selected by default when opening the media library.

- Type: `Number|Array`
- Required: No
- Platform: Web


## Extend
Expand Down
11 changes: 6 additions & 5 deletions packages/block-editor/src/components/media-placeholder/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ import { withTheme, useStyle } from '@wordpress/components';
import styles from './styles.scss';

function MediaPlaceholder( props ) {
const { mediaType, labels = {}, icon, onSelectURL, theme } = props;
const { allowedTypes = [], labels = {}, icon, onSelect, theme } = props;

const isImage = MEDIA_TYPE_IMAGE === mediaType;
const isVideo = MEDIA_TYPE_VIDEO === mediaType;
const isOneType = allowedTypes.length === 1;
const isImage = isOneType && allowedTypes.includes( MEDIA_TYPE_IMAGE );
const isVideo = isOneType && allowedTypes.includes( MEDIA_TYPE_VIDEO );

let placeholderTitle = labels.title;
if ( placeholderTitle === undefined ) {
Expand Down Expand Up @@ -52,8 +53,8 @@ function MediaPlaceholder( props ) {

return (
<MediaUpload
mediaType={ mediaType }
onSelectURL={ onSelectURL }
allowedTypes={ allowedTypes }
onSelect={ onSelect }
render={ ( { open, getMediaOptions } ) => {
return (
<TouchableWithoutFeedback
Expand Down
10 changes: 9 additions & 1 deletion packages/block-editor/src/components/media-upload/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ If allowedTypes is unset all mime types should be allowed.

- Type: `Array`
- Required: No
- Platform: Web | Mobile

### multiple

Expand All @@ -71,20 +72,23 @@ Whether to allow multiple selections or not.
- Type: `Boolean`
- Required: No
- Default: false
- Platform: Web

### value

Media ID (or media IDs if multiple is true) to be selected by default when opening the media library.

- Type: `Number|Array`
- Required: No
- Platform: Web

### onSelect

Callback called when the media modal is closed, the selected media are passed as an argument.

- Type: `Function`
- Required: Yes
- Platform: Web | Mobile

### title

Expand All @@ -93,14 +97,15 @@ Title displayed in the media modal.
- Type: `String`
- Required: No
- Default: `Select or Upload Media`
- Platform: Web

### modalClass

CSS class added to the media modal frame.

- Type: `String`
- Required: No

- Platform: Web

### addToGallery

Expand All @@ -111,6 +116,7 @@ Only applies if `gallery === true`.
- Type: `Boolean`
- Required: No
- Default: `false`
- Platform: Web

### gallery

Expand All @@ -119,13 +125,15 @@ If true, the component will initiate all the states required to represent a gall
- Type: `Boolean`
- Required: No
- Default: `false`
- Platform: Web

## render

A callback invoked to render the Button opening the media library.

- Type: `Function`
- Required: Yes
- Platform: Web | Mobile

The first argument of the callback is an object containing the following properties:

Expand Down
99 changes: 50 additions & 49 deletions 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 @@ -23,16 +23,27 @@ export const MEDIA_UPLOAD_BOTTOM_SHEET_VALUE_WORD_PRESS_LIBRARY = 'wordpress_med

export const OPTION_TAKE_VIDEO = __( 'Take a Video' );
export const OPTION_TAKE_PHOTO = __( 'Take a Photo' );
export const OPTION_TAKE_PHOTO_OR_VIDEO = __( 'Take a Photo or Video' );

export class MediaUpload extends React.Component {
constructor( props ) {
super( props );
this.onPickerPresent = this.onPickerPresent.bind( this );
this.onPickerChange = this.onPickerChange.bind( this );
this.onPickerSelect = this.onPickerSelect.bind( this );
}
getTakeMediaLabel() {
const { mediaType } = this.props;
const { allowedTypes = [] } = this.props;

const isOneType = allowedTypes.length === 1;
const isImage = isOneType && allowedTypes.includes( MEDIA_TYPE_IMAGE );
const isVideo = isOneType && allowedTypes.includes( MEDIA_TYPE_VIDEO );

if ( mediaType === MEDIA_TYPE_IMAGE ) {
if ( isImage ) {
return OPTION_TAKE_PHOTO;
} else if ( mediaType === MEDIA_TYPE_VIDEO ) {
} else if ( isVideo ) {
return OPTION_TAKE_VIDEO;
}
} return OPTION_TAKE_PHOTO_OR_VIDEO;
}
Copy link
Member

@lukewalczak lukewalczak Aug 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Generally I've updated media-upload to check if allowedTypes contains more than one type, however, I think that on mobile doesn't exist a block which is going to support both types at the same time.

EDIT: I've just noticed PR which is adding that functionality

For Picker (bottom sheet) I had to add a default label for that case OPTION_TAKE_PHOTO_OR_VIDEO, but I'm missing a proper icon in function getChooseFromDeviceIcon.

Except this I think we're ready to merge it.

Please, let me know if everything is correct.

cc: @koke

Copy link
Contributor

@pinarol pinarol Aug 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can default to image icon 'format-image' for CHOOSE_FROM_DEVICE option for cases both image&video is allowed.

Screen Shot 2019-08-23 at 18 45 03

Copy link
Member

Choose a reason for hiding this comment

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

Done!

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 admin-media is the more generic option, but I'd ask @iamthomasbishop

Copy link

Choose a reason for hiding this comment

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

If we are supporting only photo and video as media types, I think we might want to steer away from that admin-media icon because it implies audio is supported.

My preference would be to either use the existing icon (image icon as shown above by @pinarol ) or substitute in another icon, perhaps something that indicates device or smartphone. I know we have some Material icons in the repo but I'm not sure if either of these are in that group. If so, we can probably utilize them.

Copy link
Member

Choose a reason for hiding this comment

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

In that place, we are using Dashicon and currently, we are supporting only video or image, but there is a pending PR which is adding multiple types so I would like to prepare a functionality for that. The only missing item is the icon for the case where both types are supported.

Copy link
Member

Choose a reason for hiding this comment

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

So right now there is a format-image as @pinarol suggested, but if you have better idea, let me know.

Copy link

Choose a reason for hiding this comment

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

Ok, that makes sense. My suggestion would be to stay with format-image until we support additional types. I don't imagine we will support audio for a bit, but when we do, we could always switch to the icon @koke suggested.


getMediaOptionsItems() {
Expand All @@ -44,11 +55,15 @@ export class MediaUpload extends React.Component {
}

getChooseFromDeviceIcon() {
const { mediaType } = this.props;
const { allowedTypes = [] } = this.props;

const isOneType = allowedTypes.length === 1;
const isImage = isOneType && allowedTypes.includes( MEDIA_TYPE_IMAGE );
const isVideo = isOneType && allowedTypes.includes( MEDIA_TYPE_VIDEO );

if ( mediaType === MEDIA_TYPE_IMAGE ) {
if ( isImage || ! isOneType ) {
return 'format-image';
} else if ( mediaType === MEDIA_TYPE_VIDEO ) {
} else if ( isVideo ) {
return 'format-video';
}
}
Expand All @@ -61,58 +76,44 @@ export class MediaUpload extends React.Component {
return 'wordpress-alt';
}

render() {
const { mediaType } = this.props;

const onMediaLibraryButtonPressed = () => {
requestMediaPickFromMediaLibrary( [ mediaType ], ( mediaId, mediaUrl ) => {
if ( mediaId ) {
this.props.onSelectURL( mediaId, mediaUrl );
}
} );
};

const onMediaUploadButtonPressed = () => {
requestMediaPickFromDeviceLibrary( [ mediaType ], ( mediaId, mediaUrl ) => {
if ( mediaId ) {
this.props.onSelectURL( mediaId, mediaUrl );
}
} );
};

const onMediaCaptureButtonPressed = () => {
requestMediaPickFromDeviceCamera( [ mediaType ], ( mediaId, mediaUrl ) => {
if ( mediaId ) {
this.props.onSelectURL( mediaId, mediaUrl );
}
} );
};
onPickerPresent() {
if ( this.picker ) {
this.picker.presentPicker();
}
}

const mediaOptions = this.getMediaOptionsItems();
onPickerSelect( requestFunction ) {
const { allowedTypes = [], onSelect } = this.props;
requestFunction( allowedTypes, ( id, url ) => {
if ( id ) {
onSelect( { id, url } );
}
} );
}

let picker;
onPickerChange( value ) {
if ( value === MEDIA_UPLOAD_BOTTOM_SHEET_VALUE_CHOOSE_FROM_DEVICE ) {
this.onPickerSelect( requestMediaPickFromDeviceLibrary );
} else if ( value === MEDIA_UPLOAD_BOTTOM_SHEET_VALUE_TAKE_MEDIA ) {
this.onPickerSelect( requestMediaPickFromDeviceCamera );
} else if ( value === MEDIA_UPLOAD_BOTTOM_SHEET_VALUE_WORD_PRESS_LIBRARY ) {
this.onPickerSelect( requestMediaPickFromMediaLibrary );
}
}

const onPickerPresent = () => {
picker.presentPicker();
};
render() {
const mediaOptions = this.getMediaOptionsItems();

const getMediaOptions = () => (
<Picker
hideCancelButton={ true }
ref={ ( instance ) => picker = instance }
ref={ ( instance ) => this.picker = instance }
options={ mediaOptions }
onChange={ ( value ) => {
if ( value === MEDIA_UPLOAD_BOTTOM_SHEET_VALUE_CHOOSE_FROM_DEVICE ) {
onMediaUploadButtonPressed();
} else if ( value === MEDIA_UPLOAD_BOTTOM_SHEET_VALUE_TAKE_MEDIA ) {
onMediaCaptureButtonPressed();
} else if ( value === MEDIA_UPLOAD_BOTTOM_SHEET_VALUE_WORD_PRESS_LIBRARY ) {
onMediaLibraryButtonPressed();
}
} }
onChange={ this.onPickerChange }
/>
);
return this.props.render( { open: onPickerPresent, getMediaOptions } );

return this.props.render( { open: this.onPickerPresent, getMediaOptions } );
}
}

Expand Down
18 changes: 9 additions & 9 deletions packages/block-library/src/image/edit.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ class ImageEdit extends React.Component {

if ( attributes.id && attributes.url && ! isURL( attributes.url ) ) {
if ( attributes.url.indexOf( 'file:' ) === 0 ) {
requestMediaImport( attributes.url, ( mediaId, mediaUri ) => {
if ( mediaUri ) {
setAttributes( { url: mediaUri, id: mediaId } );
requestMediaImport( attributes.url, ( id, url ) => {
if ( url ) {
setAttributes( { id, url } );
}
} );
}
Expand Down Expand Up @@ -178,9 +178,9 @@ class ImageEdit extends React.Component {
} );
}

onSelectMediaUploadOption( mediaId, mediaUrl ) {
onSelectMediaUploadOption( { id, url } ) {
const { setAttributes } = this.props;
setAttributes( { url: mediaUrl, id: mediaId } );
setAttributes( { id, url } );
}

onFocusCaption() {
Expand Down Expand Up @@ -265,8 +265,8 @@ class ImageEdit extends React.Component {
return (
<View style={ { flex: 1 } } >
<MediaPlaceholder
mediaType={ MEDIA_TYPE_IMAGE }
onSelectURL={ this.onSelectMediaUploadOption }
allowedTypes={ [ MEDIA_TYPE_IMAGE ] }
onSelect={ this.onSelectMediaUploadOption }
icon={ this.getIcon( false ) }
onFocus={ this.props.onFocus }
/>
Expand Down Expand Up @@ -363,8 +363,8 @@ class ImageEdit extends React.Component {
);

return (
<MediaUpload mediaType={ MEDIA_TYPE_IMAGE }
onSelectURL={ this.onSelectMediaUploadOption }
<MediaUpload allowedTypes={ [ MEDIA_TYPE_IMAGE ] }
onSelect={ this.onSelectMediaUploadOption }
render={ ( { open, getMediaOptions } ) => {
return getImageComponent( open, getMediaOptions );
} }
Expand Down
Loading