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.

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

Use the MediaUploadCheck component before each Upload component #11924

Merged
merged 2 commits into from
Nov 17, 2018

Conversation

imath
Copy link
Contributor

@imath imath commented Nov 15, 2018

This PR should fix #11910

Description

  1. It adds a MediaUploadCheck component before the forgotten MediaUpload components used directly in some blocks :
  • within the edit toolbar buttons of the cover, file and image blocks,
  • within the Poster image setting of the video block.
  1. It also adds a MediaUploadCheck component before rendering the Block Drop Zone.

How has this been tested?

Using WordPress trunk & Gutenberg Master I've visually checked the bugs listed into the #11910 issue were fixed for the contributor role (user without the upload_files capability). I also ran successfully the unit tests suite.

Types of changes

Improves #4155 by making sure the forgotten Upload components are used after the MediaUploadCheck one.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo self-requested a review November 15, 2018 16:25
@gziolo gziolo added the [Type] Bug An existing feature does not function as intended label Nov 15, 2018
@gziolo gziolo added this to the 4.5 milestone Nov 15, 2018
@gziolo gziolo added [Feature] Media Anything that impacts the experience of managing media [Feature] Drag and Drop Drag and drop functionality when working with blocks labels Nov 15, 2018
@gziolo
Copy link
Member

gziolo commented Nov 16, 2018

It tests well. Great work on fixing all the remaining issues.

We discussed it on Slack, but I wanted to double check if you still think it is an option to include MediaUploadCheck inside MediaUpload component to ensure that it is always disabled when a user doesn't have proper capabilities? I really like how it is done for DropZone and MediaPlaceholder.

This PR should fix WordPress#11910

1. It adds a MediaUploadCheck component before the forgotten MediaUpload components used directly in some blocks :
- within the edit toolbar buttons of the cover, file and image blocks,
- within the Poster image setting of the video block.
2. It also adds a MediaUploadCheck component before rendering the Block Drop Zone.
Include some informations about how to make sure the current user has upload permissions (by wrapping the MediaUpload component into the MediaUploadCheck one)
@imath imath force-pushed the improve/upload_files-capability-checks branch from 01674f4 to 423e9b3 Compare November 17, 2018 05:29
@imath
Copy link
Contributor Author

imath commented Nov 17, 2018

@gziolo I've been looking into it and came to the conclusion that it's fine the way it is actually 😃 . Into the MediaPlaceHolder we have the MediaUpload component but we also have the DropZone and the FormFileUpload components. So I've tried to do the same UploadPermissions check into these components but never succeeded : it looks like the upload permissions are not set yet.

So I thought, as we need to wrap the DropZone, FormFileUpload and the MediaUpload components into a MediaUploadCheck one anyway, doing an extra check into MediaUpload was weird.

@gziolo gziolo merged commit 88b5033 into WordPress:master Nov 17, 2018
@gziolo
Copy link
Member

gziolo commented Nov 17, 2018

Let's go with the current approach and see how it work. There are so many different use cases where the check for upload permissions is necessary that it's hard to find a perfect approach. We can always iterate on it. Thanks for fixing all remaining bugs I could find.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Drag and Drop Drag and drop functionality when working with blocks [Feature] Media Anything that impacts the experience of managing media [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve 'upload_files' capability checks in the editor
2 participants