-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[Inserter - Media tab]: Upload Openverse images when inserted #48501
[Inserter - Media tab]: Upload Openverse images when inserted #48501
Conversation
Size Change: -94 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Did you try the cache hack? Which images can't be uploaded? |
Let's shared the links that can't be uploaded for testing and see if that hack works with them |
Some images that cant be uploaded |
Thanks for sharing these links @carolinan ! I tried with some of them and the workaround from @ellatrix doesn't seem to work and we still can't upload the image. So it seems we need to make a decision about how to handle this. Could the current approach with inserting the image block with the external source and a warning work? |
Flaky tests detected in fa60819. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4292589296
|
// don't need to check for allowed mime types, as they are used | ||
// for restricting uploads for this media type and not for | ||
// inserting media from external sources. | ||
if ( category.isExternalResource ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we upload the images from external sources, so we have to check the allowedMimeTypes
setting.
@@ -132,12 +19,6 @@ function MediaList( { | |||
label = __( 'Media List' ), | |||
} ) { | |||
const composite = useCompositeState(); | |||
const onPreviewClick = useCallback( | |||
( block ) => { | |||
onClick( cloneBlock( block ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the cloneBlock
call inside the MediaPreview
callback.
'block-editor-inserter__media-list__item-preview-options__popover', | ||
}; | ||
|
||
function MediaPreviewOptions( { category, media } ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MediaPreviewOptions
component had no changes here. Just moved it together with MediaPreview
component.
( select ) => select( blockEditorStore ).getSettings().mediaUpload, | ||
[] | ||
); | ||
const onMediaInsert = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change in this file, alongside a couple of checks of isInserting
to show conditionally the spinner
and the MediaPreviewOptions
.
The remaining code of this component was moved as it was in this separate file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that some of these callbacks could be move to be an actual action in block-editor
store. Not sure exactly what API it would have but it feels heavy to do all these async calls in the components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a look at this in a follow up too, if that's okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some generic comments, maybe the most "concerning" one is the truncating but this is working well in my testing.
packages/block-editor/src/components/inserter/media-tab/media-preview.js
Show resolved
Hide resolved
( select ) => select( blockEditorStore ).getSettings().mediaUpload, | ||
[] | ||
); | ||
const onMediaInsert = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that some of these callbacks could be move to be an actual action in block-editor
store. Not sure exactly what API it would have but it feels heavy to do all these async calls in the components.
let truncatedTitle; | ||
if ( title.length > MAXIMUM_TITLE_LENGTH ) { | ||
const omission = '...'; | ||
truncatedTitle = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think manipulating strings like that is not great. It has a potential of not working properly depending on locals... Can we truncate using CSS instead? Or use some prior work here. (Truncate component maybe though I've never used this one)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me handle this in a follow up outside of this PR and 6.2, to make sure we don't introduce any regression at this point of the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uploading seems to work well:
upload.mp4
Attempting to insert an incompatible image I see this:
The wording of the Snackbar didn't clearly indicate whether the image had been inserted or not. If anything the first sentence sets a tone that it wasn't inserted. It's also a lot of jargony content that is difficult to parse quickly (before the Snackbar expires 🙈).
Do we have an idea of how common this is? If it's super rare then it might be ok to merge and refine later. But if it's common then it'd be nice to address.
An alternative idea:
Image inserted, but unable to upload to media library [Learn more]
Clicking Learn more would open a modal with the full description. What do you think?
@ntsekouras @carolinan Strange, I have no problem uploading this. Are you trying in chrome? |
It's just the one @carolinan mentioned above (search 'Apple'). I didn't encounter any others, but I also didn't go searching for them. |
I am using Chrome Version 110.0.5481.177, macos, from localhost. |
@jameskoster I updated to use a Modal. Can you take a look again? Thanks! |
packages/block-editor/src/components/inserter/media-tab/media-preview.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inserter/media-tab/media-preview.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In terms of design this looks good to me. The modal is working and is warranted for external images imo due to the potential legal consequences.
Might I suggest changing the copy slightly:
I would change to:
GDPR might not mean anything to a user. |
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: fac5049 |
* [Inserter - Media tab]: Upload Openverse images when inserted * extract to separate hook * Check all media categories against `allowedMimeTypes` * add a spinner when is uploading image and extract Preview in separate file * Add extra safeguard agains category fetching request * add modal if the image fails to upload * rename some props * update copy
Not for 6.2, but let's connect later with the other flows around external images (#46014) and the messaging we use to communicate to users the potential problems with them. The dialog text refined here could be useful for the publish flow panel messaging, for example. |
What?
Fixes: #48394
This PR uploads the images from Openverse before inserting them. If an image cannot be uploaded to media library due to CORS issues, we insert the Image block with the external URL and show a warning with the possible implications.
I also had to partially revert this PR, because now we upload the images from external sources, so we have to check the
allowedMimeTypes
setting. I haven't added the requests to preload again, so that means that we have a small delay to decide whether to render themedia
tab or not, similar to thepatterns
one.Tasks
Testing Instructions
Openverse
category in Media tab