Refactor the MediaUpload components to check upload permissions by checking the upload handler existence #14548
Conversation
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
gziolo,
noisysocks and
talldan
as code owners
| } else if ( isVideo ) { | ||
| instructions = __( 'Drag a video, upload a new one or select a file from your library.' ); | ||
| } | ||
| } else if ( ! hasUploadPermissions && onSelectURL ) { |
There was a problem hiding this comment.
The fallback instructions here are not that useful IMO and the "role" is not the only reason the user can't upload. In a serverless gutenberg instance, upload is never allowed for instance.
There was a problem hiding this comment.
Hi @youknowriad, I am not sure if the design will look good if the placeholder does not contain a message.
Maybe we can use a simple message like:
"Link to an audio file, using the options bellow".
There was a problem hiding this comment.
Actually, the design felt better to me without any instructions as there was only a unique action.
There was a problem hiding this comment.
I do like the simplicity of the single button action.
One concern is that a user might be versed in WordPress and be an admin on one site, and a contributor on another site. This user might be confused as to why the Upload button is gone here, and a description would mitigate that. It also somewhat helps explain this image placeholder, which is a somewhat new flow compared to traditional editors — here you insert the block first, then pick the image.
At the same time, it is extremely likely that this image placeholder will go through a number of additional iterations, such as making it resizable like an image, or allow you to show the media library inline for one-click image picking. In that vein it doesn't seem too fruitful to hold up solid improvements to hyper-optimize and edgecase for an interface that is going to change anyway.
In other words: descriptions are helpful, but okay to go with this for now given it will be revisited.
| @@ -146,7 +145,7 @@ export class MediaPlaceholder extends Component { | |||
| let instructions = labels.instructions || ''; | |||
| let title = labels.title || ''; | |||
|
|
|||
| if ( ! hasUploadPermissions && ! onSelectURL ) { | |||
| if ( ! mediaUpload && ! onSelectURL ) { | |||
| instructions = __( 'To edit this block, you need permission to upload media.' ); | |||
There was a problem hiding this comment.
Hi @youknowriad,
Given what you referred:
"role" is not the only reason the user can't upload. In a serverless gutenberg instance, upload is never allowed for instance.
Maybe this message should also be updated.
There was a problem hiding this comment.
But In this particular case, the message is right because if you're not allowed to paste URLs (there's no onSelectURL) the only way to add media is mediaUpload but if mediaUpload is not available, it means you don't have the rights to upload as in theory if you use the component without passing onSelectURL it means you at least provided a mediaUpload handler which means the only reason this could happen is if there's no permissions.
I get we can update with a more generic Unable to edit this block or something like that to simplify though.
| } else if ( isVideo ) { | ||
| instructions = __( 'Drag a video, upload a new one or select a file from your library.' ); | ||
| } | ||
| } else if ( ! hasUploadPermissions && onSelectURL ) { |
There was a problem hiding this comment.
Hi @youknowriad, I am not sure if the design will look good if the placeholder does not contain a message.
Maybe we can use a simple message like:
"Link to an audio file, using the options bellow".
There was a problem hiding this comment.
Thank you for the clarifications @youknowriad. For the code changes point of view, I think we can merge this after the rebase.
youknowriad
force-pushed
the
update/drop-core-store-block-editor
branch
from
da0bb10 to
eb32b58
Compare
youknowriad
added
Needs Design Feedback
packages/block-editor/src/components/media-placeholder/index.js
Outdated
Show resolved
Hide resolved
…ecking the upload handler existence
youknowriad
force-pushed
the
update/drop-core-store-block-editor
branch
from
63b0e15 to
c28f11f
Compare
| @@ -4903,8 +4902,7 @@ | |||
| "ansi-regex": { | |||
| "version": "2.1.1", | |||
| "bundled": true, | |||
| "dev": true, | |||
| "optional": true | |||
There was a problem hiding this comment.
I suspect this might have been generated with a not-latest version of NPM? I see all of these optional properties added back in master when running the (prescribed) latest version of NPM (6.9.0).
Related #14043
In a generic block editor module, the existence of an upload handler should be sufficient in checking that upload is allowed or not. This means if the user don't have upload permissions, we should just omit the upload handler in the settings to fallback the experience.
This means we don't need the "core-data" dependency in the block-editor package to perform this check.
Note that core-data is still used but when both this PR and #14387 land, there will be no core-data usage anymore.
Testing instructions