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

Fix max upload size error message #4203

Merged
merged 1 commit into from May 22, 2018

Conversation

@jorgefilipecosta
Member

jorgefilipecosta commented Dec 29, 2017

Description

This PR aims to fix #3984. It improves media upload error handling, by showing a generic message when the upload fails, and by validating the max file size before starting the upload so we are not wasting resources try to send an invalid file to the server.

How Has This Been Tested?

Add an image block. Try to upload an image bigger than max file size upload limit. the value can be changed by adding this lines "php_value upload_max_filesize 3M / php_value post_max_size 3M" in .htaccess.
See that the correct error notice appears.

Screenshots (jpeg or gifs if applicable):

screen shot 2018-01-15 at 22 43 53

screen shot 2018-01-15 at 22 44 11

Types of changes

Added an onError handler to media handler. Used the handler to validate max file size and generic upload problems.
Passed max upload size from in client assets to allow max file size validation.
Added the ability for blocks to display error notices.

@iseulde

This comment has been minimized.

Member

iseulde commented Dec 31, 2017

Nice pre checking! Would it perhaps be nice to display the error on the block itself (locally), instead of at the top (globally)?

@karmatosed

This comment has been minimized.

Member

karmatosed commented Jan 3, 2018

Right now all our error/success messages go in one place, the top. I think having this also go there is for now right. We absolutely could consider what messages work for contextual, but that feels a different issue and shouldn't block this going in as is.

@iseulde

This comment has been minimized.

Member

iseulde commented Jan 3, 2018

@karmatosed We currently have some error messages that are block-specific that go in the block, such as JS errors and externally modified messages.

@karmatosed

This comment has been minimized.

Member

karmatosed commented Jan 4, 2018

Hmm, isn't it only JS errors though or external? I'm thinking from a UX perspective the 'message place' works at top for user information.

So we can track this, do we have a record/list of all error messages and where they output? Would be a good exercise in seeing we're being consistent.

@iseulde

This comment has been minimized.

Member

iseulde commented Jan 4, 2018

E.g. create a list block, click the ellipsis, edit in HTML, remove the list HTML and add something else, then try to go back to visual. There will be a message with three action buttons.

@iseulde

This comment has been minimized.

Member

iseulde commented Jan 10, 2018

Would be good to have this. Ran into it with paste too.

@@ -421,6 +422,7 @@ export class BlockListBlock extends Component {
id={ block.uid }
isSelectionEnabled={ this.props.isSelectionEnabled }
toggleSelection={ this.props.toggleSelection }
createErrorNotice={ this.props.createErrorNotice }

This comment has been minimized.

@iseulde

iseulde Jan 10, 2018

Member

Code-wise it also seems cleaner to display the error within the block? This error does not apply to the whole page.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 10, 2018

Member

Thank you for the feedback @iseulde, I will propose a version with a block-specific error 👍

This comment has been minimized.

@iseulde

iseulde Jan 10, 2018

Member

Awesome, thanks! :)

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jan 11, 2018

Hi @karmatosed, @iseulde, I added a new mechanism so we can test a different approach using block level error notices. The errors are different from the existing block parsing error. In this case, the block should continue to work normally e.g in the gallery even if one image failed the upload I should be able to see and manage the other images or upload a different one. I will update screenshots so we can see the new approach.

@iseulde

This comment has been minimized.

Member

iseulde commented Jan 15, 2018

Just wondering out loud... What if the error notice is part of the block edit component, and we just have a standard class for the error notice (kind of like it is done in WP core now). Or it is a component similar to Editable etc. Would that not make the code simpler? Or does the error notice need to appear outside the block edit boundary? I think the error makes sense to display as close to where the state lives, if that makes sense, not only code wise, but also visually. The error was triggered inside the upload part of the image block. Now it kind of feels like its a block on its own.

@iseulde

This comment has been minimized.

Member

iseulde commented Jan 15, 2018

Just ideas

screen shot 2018-01-15 at 13 25 37

screen shot 2018-01-15 at 13 27 11

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jan 15, 2018

Just wondering out loud... What if the error notice is part of the block edit component, and we just have a standard class for the error notice (kind of like it is done in WP core now). Or it is a component similar to Editable etc. Would that not make the code simpler?

The problem with this approach is that each block would need to implement the management of the notices. E.g: keep the state of which notices are being shown, and provide functions to dismiss them and then pass this functions down to the generic component as props. This notices management logic would be repeated across the blocks needing notices. Right now it is not repeated e.g blocks just have a function to throw notices and then they don't have to handle them.

The error was triggered inside the upload part of the image block. Now it kind of feels like its a block on its own.

We can try to improve the visual, instead of rendering directly we can pass a prop that contains the rendered notices. The blocks can then choose to place them inside some specific part, for example, render inside the placeholders like in your first example ( I will try to provide a sample). But in the non-empty states ( e.g: a gallery with images where the upload of a new image failed.) the notices would still look similar to the example we have now.

@iseulde

This comment has been minimized.

Member

iseulde commented Jan 15, 2018

But in the non-empty states ( e.g: a gallery with images where the upload of a new image failed.) the notices would still look similar to the example we have now.

That makes sense.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jan 15, 2018

Hi @iseulde, this PR got updated so the notices feel more part of the block. Thank you for the feedback 👍 I added screenshots to the new design. The mechanism is still easy to use for block creators e.g: notices can be added with online to show them and another line to create a new message.

@@ -62,6 +62,10 @@ registerBlockType( 'core/gallery', {
},
},
supports: {
notices: true,

This comment has been minimized.

@iseulde

iseulde Jan 18, 2018

Member

Why is this needed? I'm not sure I understand this.

/** @inheritdoc */
render() {
if ( ! hasBlockSupport( this.props.name, 'notices' ) ) {

This comment has been minimized.

@iseulde

iseulde Jan 18, 2018

Member

From the previous comment, can't we always add the notices props?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 18, 2018

Member

Yes, it would be an option, the supports notices is just for the property to be opt-in, e.g: blocks just receive if they ask for it.

*
* @param {string} id Id of the notice to remove.
*/
removeNotice( id ) {

This comment has been minimized.

@iseulde

iseulde Jan 18, 2018

Member

What happens when a block with a notice disappears? Does it just stay around here in the state? This is why I think it would be beneficial for the state to be local, not global.

This comment has been minimized.

@iseulde

iseulde Jan 18, 2018

Member

In other words, why do we need to keep a global list?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jan 18, 2018

Member

Notices is a HoC around blocks, it would be the same as doing something like withNotices( GalleryBlock ); that adds notices property to gallery block. I can easily switch to this version. The only disadvantage is that blocks would need to be React components to use it and the current approach also allows blocks to take advantage even if their edit method is a function. When a block with notices is removed, the HoC around it will also be removed and its state is deleted.
Similar to happens when we use withAPIData the HoC also has its own state when a block it wraps is removed its own state is also deleted.

@iseulde

This comment has been minimized.

Member

iseulde commented Jan 18, 2018

Just wondering out loud... What if the error notice is part of the block edit component, and we just have a standard class for the error notice (kind of like it is done in WP core now). Or it is a component similar to Editable etc.

I understand your point here (#4203 (comment)), but could it then still not be a simple embedded reusable component within a block, similar to e.g. controls and editable?

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jan 18, 2018

I understand your point here (#4203 (comment)), but could it then still not be a simple embedded reusable component within a block, similar to e.g. controls and editable?

Maybe it is possible, but I'm a little mind blocked and I'm not seeing the way :(
We can have a component that renders the notices, but the notices can be added from the outside of that component e.g in the upload buttons. So the state cannot be managed in the component that renders the notices as we can add notices from the outside. In the editable case, we pass some attribute as prop and functions to update that attribute but the state of the editable is not updated from the outside.

So all blocks with notices would need to keep notices in their own state right? And provide methods to add and remove them? Or is there another alternative in your point of view?
What we can do easily is make our HoC not render the notices.UI and pass an array of notices, methods to add and removed them. Blocks would be responsive to render them (can do that easily using the NoticeList component.
As pointed in another comment we can also make the HoC use explicit e.g blocks would use withNotices( GalleryBlock );

@karmatosed

This comment has been minimized.

Member

karmatosed commented Jan 18, 2018

Visually I'd say keep as close to the existing message styling as possible. So I'd go with the first option @iseulde linked here:

https://user-images.githubusercontent.com/4710635/34942417-dd7d4f16-f9f7-11e7-9dbe-2e32181ab2d7.png

I don't think just colouring it red works as that seems easy to miss for color translation issue. I'll think on this as may have some feedback in addition. However, I think it's a good path to move on.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Feb 6, 2018

Hi @karmatosed, the design was updated to match the image provided. It now looks like this:
image

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Mar 13, 2018

Hi @karmatosed, the PR was rebased, lots of changes happened in parallel, so the code suffered some changes to be updated. The design changes required were already addressed and I don't expect design changes with this rebase.

This PR adds the error notices, to gallery and image blocks.

If we accept this apporach similar changes will happen to drop down events when no block exists yet, and to other blocks with uploads e.g: video. Doing all the changes here whould make this PR even bigger and make rebases even more refrequent.

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Mar 13, 2018

@karmatosed

This comment has been minimized.

Member

karmatosed commented Apr 26, 2018

@jorgefilipecosta I want to get a review on this so we can actually get it in, are the conflicts able to be resolved?

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Apr 26, 2018

Hi @karmatosed, the conflicts are resolved and a new rebase was performed. This PR touches many parts of the APP so at any time new conflicts may be created.
It should be ready for review in the images and gallery block. I will perform some more testing as it was a huge rebase.

@karmatosed

Thanks for rebase @jorgefilipecosta.

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Apr 27, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 1, 2018

Hi @iseulde, from the technical point of view this code was also updated we now make use of a HOC, I would appreciate if you could have a new look :)

* @param {Array} filesList List of files.
* @param {Function} onFileChange Function to be called each time a file or a temporary representation of the file is available.
* @param {string} allowedType The type of media that can be uploaded.
* @param {Object} $0 Parameters object passed to the function.

This comment has been minimized.

@iseulde

iseulde May 1, 2018

Member

Interesting notation! :)

maxUploadFileSize,
onError = noop,
onFileChange,
} ) {

This comment has been minimized.

@iseulde

iseulde May 1, 2018

Member

Nice to have an options object here!

this.removeNotice = this.removeNotice.bind( this );
this.state = {
notices: [],

This comment has been minimized.

@iseulde

iseulde May 1, 2018

Member

Wouldn't it be simpler to store notices by ID here?

this.state = {
  1: {},
  2: {},
}

That also makes it easier to add and delete.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 1, 2018

Member

I used an array to follow the same logic we use on the global notices. I wanted to use the same that is being used globally.

* @return {Component} Wrapped component.
*/
export default createHigherOrderComponent( ( BlockEdit ) => {
return class WrappedBlockEdit extends Component {

This comment has been minimized.

@iseulde

iseulde May 1, 2018

Member

Curious: why enhance the BlockEdit component and not let block authors enhance their own edit component or a child component of that? The whole BlockEdit does not need to rerender on a state change, right? Notices are only shown within the edit component of the block, or am I mistaken?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 1, 2018

Member

Sorry, I missed the update to the component name during the changes, it should have been called OriginalComponent instead of BlockEdit.
withNotices is a generic HOC that can be passed to any react component and it is not being used against BlockEdit we use it inside the blocks edit because as you said notices are only shown within the edit component of the block.

@@ -236,6 +247,7 @@ class GalleryBlock extends Component {
</PanelBody>
</InspectorControls>
<ul className={ `${ className } align${ align } columns-${ columns } ${ imageCrop ? 'is-cropped' : '' }` }>
{ noticesUI }

This comment has been minimized.

@iseulde

iseulde May 1, 2018

Member

Can this be a child of ul?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 1, 2018

Member

It should be outside of the ul during the rebase I made a mistake it was corrected. Thank you!

@iseulde

This comment has been minimized.

Member

iseulde commented May 1, 2018

The main question for me is #4203 (comment). I don't understand why the component need to be so high up the tree with passing a notices object etc.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 1, 2018

Hi, @iseulde thank you a lot for the review. The component was not high up in the tree, but as I used BlockEdit as the name of the component passed to the HOC, it looked like it was being used high up in the tree. The name was corrected.

@gziolo

This comment has been minimized.

Member

gziolo commented May 10, 2018

@jorgefilipecosta - can you update this PR and make it mergeable? What are remaining blockers in here?

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 10, 2018

Hi @gziolo, thank you for checking this PR, I rebased it. I think we don't have blocking points, from the design perspective it was approved. I would like to have final approval from the code perspective because we are implementing and exposing a local notices mechanism for block creators (which may not be trivial to change later).

@iseulde

iseulde approved these changes May 14, 2018 edited

Just a few questions... looks great otherwise. I wish there wasn't so much stuff passed and that the API were a bit simpler.

There's also a failing test.

const classes = classnames( 'components-placeholder', className );
return (
<div { ...additionalProps } className={ classes }>
{ !! notices && notices }

This comment has been minimized.

@iseulde

iseulde May 14, 2018

Member

Why is !! notices needed? Can't it be null?

@@ -195,6 +198,13 @@ export default class GalleryEdit extends Component {
</BlockControls>
);
const noticesUI = notices.noticeList.length > 0 &&
<BlockNotices

This comment has been minimized.

@iseulde

iseulde May 14, 2018

Member

I'm just wondering here... Could the BlockNotices be a prop of this component that is added by withNotices. Or would there be any other way to avoid passing all this stuff (less for the block author to worry about).

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 16, 2018

Hi, @iseulde thank you for your review! I followed your suggestion, and now withNotices adds a UI prop so we make the usage easier and we don't have to pass many props around. If the user wants a custom UI it is still possible to have one.

<OriginalComponent
noticeList={ this.state.noticeList }
noticeOperations={ this.noticeOperations }
noticeUI={

This comment has been minimized.

@iseulde

iseulde May 16, 2018

Member

This seems nicer! Wondering if it could make it clear things up even more if noticeOperations are properties on noticeUI? Do you think that would be strange?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 16, 2018

Member

Related with #4203 (comment) although we pass a noticeUI I wanted it to be a totally optional prop and allow components to ignore it if they just want the notices without relying on the default UI we provide.

render() {
return (
<OriginalComponent
noticeList={ this.state.noticeList }

This comment has been minimized.

@iseulde

iseulde May 16, 2018

Member

Does that still need to be passed?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta May 16, 2018

Member

It is not being used, but passing it allows a user of the HOC to create a different UI and not rely on the provided noticeUI.

@iseulde

Looks good to me!

@mirka mirka referenced this pull request May 17, 2018

Closed

Implement File block #6805

11 of 11 tasks complete
Added noticesUI to withNotices HOC. (+1 squashed commit)
Squashed commits:
[5cceb909a] Added maximum file size validation to media upload; Passed maxUploadSize client-assets.

This change makes client size validation of the file size, avoiding spending time uploading invalid files to server. It also shows a friendly error message if the file size validation fails.

@jorgefilipecosta jorgefilipecosta merged commit deabbc3 into master May 22, 2018

2 checks passed

codecov/project 46.41% (-0.02%) compared to cd5779d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/max-upload-size-error-message branch May 22, 2018

@danielbachhuber danielbachhuber added this to the 3.0 milestone May 22, 2018

mirka added a commit to mirka/gutenberg that referenced this pull request May 26, 2018

Fix drag-and-drop uploading
Accommodates the changes made to editorMediaUpload() arguments in WordPress#4203

@mirka mirka referenced this pull request May 27, 2018

Merged

Check allowed mime types before uploading media #6968

4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment