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.

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

Add error notices mechanism directly to media placeholder #6957

Merged
merged 1 commit into from Jun 18, 2018

Conversation

Projects
None yet
3 participants
@jorgefilipecosta
Member

jorgefilipecosta commented May 25, 2018

This change makes upload error notices available in all blocks.

I noticed while adding the error messages to other blocks(besides gallery and image) that the best way to not duplicate code and make block creation easier would be to have the messages being directly handled in media placeholder as most blocks only allow uploading via media library (that already as this errors via core functionality) and via media placeholders that now have this messages too.

Making error notices available directly in media placeholder makes all blocks even the ones created by the community take advantage of our upload error notices system.
Blocks can opt-out of this behavior and manage the notices themselves like the gallery is doing.
Gallery manages the upload error notices, because in the media placeholder for a gallery we may select many files, some may have been uploaded correctly and in some, we may have errors. When some file is uploaded correctly we stop displaying the placeholder, so if a file upload went wrong we would not display the notice. Given that the notices for the gallery are managed at the block level that problem does not happen.

How has this been tested?

Upload files with sizes bigger than the maximum file size allowed in all blocks with media placeholders (gallery, image, audio, video, cover-image) and verify the message is correctly displayed.
Upload files with sizes lower than the maximum file size allowed in all blocks with media placeholders (gallery, image, audio, video, cover-image) and verify the upload went ok and not error message is shown.
On the gallery block upload, an allowed and an unallowed file, verify the uploaded file is showed correctly and an error message is shown above it regarding the error on the unallowed file.

@jorgefilipecosta jorgefilipecosta self-assigned this May 25, 2018

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core May 29, 2018

@jorgefilipecosta jorgefilipecosta changed the title from Add error notices directly to mechanism to media placeholder to Add error notices mechanism directly to media placeholder May 31, 2018

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 4, 2018

Related #6154

@tofumatt

I think this is the right approach and the path is right but we could maybe tidy up the API and some minor variable naming stuff. Do you think the disable prop approach makes a bit more sense or is that kind of "set disable to true" approach universally rejected in Gutenberg?

@@ -63,13 +64,18 @@ class MediaPlaceholder extends Component {
}
onFilesUpload( files ) {
const { onSelect, type, multiple, onError } = this.props;
const { onSelect, type, multiple, onError = noop, maxUploadErrorMessages = true, noticeOperations } = this.props;

This comment has been minimized.

@tofumatt

tofumatt Jun 11, 2018

Member

maxUploadErrorMessages feels like a bit of an odd prop name for this. It's effectively a setting and this prop name makes it feel a bit ambiguous–like it could be an array of messages. Maybe enableMaxUploadLimitErrorMessages or just enableMaxUploadErrorMessages?

Just so it feels more like a setting.

I guess because it's true by default and setting enableMaxUploadLimitErrorMessages={false} is a weirder API we could do disableMaxUploadErrorMessages and set it to false by default. It would make the API for components that change the default setting nicer.

const setMedia = multiple ? onSelect : ( [ media ] ) => onSelect( media );
editorMediaUpload( {
allowedType: type,
filesList: files,
onFileChange: setMedia,
onError,
onError: ( errorMsg ) => {

This comment has been minimized.

@tofumatt

tofumatt Jun 11, 2018

Member

Could this be errorMessage? Msg isn't as easy to scan and I don't think we need the abbreviations.

@@ -94,7 +101,7 @@ class MediaPlaceholder extends Component {
label={ labels.title }
instructions={ sprintf( __( 'Drag %s, upload a new one or select a file from your library.' ), labels.name ) }
className={ classnames( 'editor-media-placeholder', className ) }
notices={ notices }
notices={ <Fragment>{ additionalNotices } { noticeUI }</Fragment> }

This comment has been minimized.

@tofumatt

tofumatt Jun 11, 2018

Member

Is the space in here needed?

@@ -211,8 +211,9 @@ class GalleryEdit extends Component {
onSelect={ this.onSelectImages }
accept="image/*"
type="image"
maxUploadErrorMessages={ false }

This comment has been minimized.

@tofumatt

tofumatt Jun 11, 2018

Member

Yeah, what I said before about using disable as a prop instead I think makes the most sense. If we changed the prop to be disableMaxUploadErrorMessages with a false value by default this would look like:

<MediaPlaceholder
	[...]
	accept="image/*"
	type="image"
	disbaleMaxUploadErrorMessages
	[...]
/>

which I think is a bit clearer.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 18, 2018

Thank you for the review @tofumatt all the feedback was applied.

@@ -63,13 +64,18 @@ class MediaPlaceholder extends Component {
}
onFilesUpload( files ) {
const { onSelect, type, multiple, onError } = this.props;
const { onSelect, type, multiple, onError = noop, disbaleMaxUploadErrorMessages = false, noticeOperations } = this.props;

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 18, 2018

Member

Typo: disbaleMaxUploadErrorMessages

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 18, 2018

Member

Typo fixed, thank you for the catch!

@danielbachhuber danielbachhuber added this to the 3.1 milestone Jun 18, 2018

@tofumatt

Looks good; I left some points I think could be improved a bit but good to go with comments addressed or ignored-if-thought-incorrect.

onError,
onError: ( errorMessage ) => {
onError( errorMessage );
if ( ! disableMaxUploadErrorMessages ) {

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

I'd be tempted to add a one-line comment here explaining why we check for a false "disabled" prop as without seeing it being used by the component it might be a bit strange to read. Maybe you could link to my comment in the PR or just say:

// We use a prop named `disable`, set to `false` by default, because it makes for a nicer
// component prop API. eg:
//
// <SomeComponent disableMaxUploadErrorMessages />
// 
// instead of:
// 
// <SomeComponent enableMaxUploadErrorMessages={ false } />

Just a thought.

This comment has been minimized.

@tofumatt

tofumatt Jun 18, 2018

Member

Also I'd find this check a bit more readable if it were disableMaxUploadErrorMessages === false because:

  1. I think it communicates the intent a bit more clearly
  2. It's a boolean anyway and the default is false so it's fine
@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 18, 2018

(@danielbachhuber's request for change was made–fixing a typo–so I'ma clear his request for changes.)

Request for review was to fix typo, which is now fixed.

Add error notices mechanism to media placeholder; Makes upload error …
…notices available in all blocks.

Making error notices available directly in media placeholder makes all blocks even the ones created by the community take advantage of our upload error notices system.
Blocks can opt-out of this behavior and manage the notices themselves like the gallery is doing.
Gallery manages the upload error notices, because in the media placeholder for a gallery we may select many files, some may have been uploaded correctly and in some, we may have errors. When some file is uploaded correctly we stop displaying the placeholder, so if a file upload went wrong we would not display the notice. Given that the notices for the gallery are managed at the block level that problem does not happen.

@jorgefilipecosta jorgefilipecosta merged commit a5ceee2 into master Jun 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/error-notices-mechanism-media-placeholder branch Jun 18, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 18, 2018

Thank you for the reviews @danielbachhuber and @tofumatt!

jorgefilipecosta added a commit that referenced this pull request Jun 21, 2018

Fix bug where server side upload errors disappear automatically (#7386)
Unfortunately, the general approach to add upload error messages to all blocks without changing them does not seems possible, it created a regression as described in #6957.

What happens is that when we upload we create a temporary blob URL for the file so that the placeholder disappears and the uploaded file appears immediately. In this case, the placeholder is unmounted. If we get an error message from the server e.g.: not something we can validate on the client, we set an error message but the placeholder is already not visible. Then a new placeholder is rendered, but its state is empty and the error message we set is not used anywhere.
This makes impossible to handle the notices in the media placeholder and forces them to be managed at the block level.

This commit does the following changes:
Reverts the changes by #6957 in the gallery, image, and media-placeholder.

Adds upload error notices to video, audio and cover image.

Solves an existing bug in the video, audio, and cover image blocks where if the upload failed, we continued with a blob URL set.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment