Fix media placeholder to only activate for media objects. #66986
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tellthemachines
added
[Type] Bug
tellthemachines
requested review from
getdave,
talldan,
richtabor,
ramonjd and
t-hamano
|
Size Change: +81 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
ramonjd
reviewed
Nov 14, 2024
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This PR fixes the bug where I can't drag blocks onto the cover block. The media placeholder isn't shown for non-media blocks. 👍🏻 Maybe you can set me straight on something though: are the items in the "Media" columns already uploaded? If so, maybe dragging and dropping them should create the appropriate blocks rather than trigger the upload placeholder. Don't want to derail progress on this, so if that makes sense it could be a good follow up. |
Yes if they appear in the "Images" "Video" or "Audio" sections, but not if they're under "Openverse". In that case, they get auto-uploaded when they're dragged in.
For Cover, the intention is that the images get added as background images, not that they're added as Image blocks inside it. I guess what we could do is change the message in the media placeholder if it's being shown by a Cover block. |
talldan
reviewed
Nov 14, 2024
Comment on lines
291
to
294
| async function onMediaDrop( HTML ) { | ||
| const blocks = pasteHandler( { HTML } ); | ||
| return await handleBlocksDrop( blocks ); | ||
| } |
There was a problem hiding this comment.
Nit - it might good to make it consistent with the docs for onMediaDrop (though my second comment would make this unnecessary)
Suggested change
| async function onMediaDrop( HTML ) { | |
| const blocks = pasteHandler( { HTML } ); | |
| return await handleBlocksDrop( blocks ); | |
| } | |
| async function onMediaDrop( media ) { | |
| const blocks = pasteHandler( { media } ); | |
| return await handleBlocksDrop( blocks ); | |
| } |
Comment on lines
29
to
33
| /** | ||
| * The function is called when dropping media into the `DropZone`. | ||
| * It receives the media being dropped as an argument. | ||
| */ | ||
| onMediaDrop?: ( media: string ) => void; |
There was a problem hiding this comment.
I'm not sure about the idea of this new onMediaDrop event handler in the components package. It feels like something that's handling a very specific block editor use case. It still receives an HTML string, which might be confusing. Some consumers of the component might be confused about why it doesn't work when they're dragging media items, and it'd take some digging to find they have to set the dataTransfer type to media.
Looking at precedence, this component doesn't have an onBlocksDrop, that's handled via onDrop (in useBlockDropZone, and this custom dataTransfer type could work similarly?
There was a problem hiding this comment.
Hmm do you mean leveraging onDrop for the media type instead of adding a custom handler? I can give it a go!
There was a problem hiding this comment.
I updated to use onDrop as well as the default type which avoids any change to the components package. I think it's working well!
There was a problem hiding this comment.
I'm not really sure what the right value is for the dataTransfer type but using onDrop in this way looks better, thanks for changing it!
There was a problem hiding this comment.
Yeah media is nice because it's explicit, but here we just need something to distinguish between media and non-media; whatever value we give it, DropZone will interpret it as default anyway.
|
Flaky tests detected in c35aa68. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11848710338
|
ramonjd
approved these changes
Nov 15, 2024
There was a problem hiding this comment.
Looking good! 🚀
Dragging blocks onto the cover block works a treat.
Here's what I'm seeing in zoomed out mode (for media items):
Kapture.2024-11-15.at.13.00.47.mp4
Not for this PR, but I think one day it'd be a good user experience to change the message from "Drop files to upload" to something else when already-uploaded media can't be dropped.
talldan
approved these changes
Nov 15, 2024
There was a problem hiding this comment.
Tested a bunch of things, and it's working great now:
- When in placeholder mode, blocks insert above and below as expected
- In placeholder mode, media can be dragged in to the cover and set as the background
- With a background, blocks/patterns from the inserter can be dragged in as inner blocks
- Files and other media can still be dragged in to replace the background
Nice work!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #66422.
Currently, any block or pattern being dragged from the inserter onto a Cover block activates its media placeholder. This PR changes the behaviour to activate the placeholder only if the object being dragged is an image, video or audio. (Audio won't work for Cover, but the same placeholder serves Audio blocks as well, so it needs to work for them.)
InserterDraggableBlocks, and sets its dataTransfer type to 'media' instead of 'html' if it's a media object.onMediaDrophandler type toDropZoneComponent, which is used if the dataTransfer type is 'media'MediaPlaceholderto pass anonMediaDrophandler toDropzoneinstead ofonHTMLDrop.Testing Instructions
Note that currently there's an unrelated bug in the Cover block edit function that's preventing images and videos from properly being added to the block, but at least the placeholder should be activated 😅
I'll look into that other bug as a separate task.
Testing Instructions for Keyboard
Screenshots or screencast
drag_onto_cover.mp4