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.

Sign up for GitHub

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 media placeholder to only activate for media objects. #66986

Merged
merged 4 commits into from
Nov 15, 2024
+16 −7
Merged

Fix media placeholder to only activate for media objects. #66986

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b20feeb
Fix media placeholder to only activate for media objects.
tellthemachines Nov 14, 2024
8fae30f
Update e2e string
tellthemachines Nov 14, 2024
ee7f1c2
Use default type instead of media
tellthemachines Nov 15, 2024
c35aa68
update e2e
tellthemachines Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/block-editor/src/components/inserter-draggable-blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,15 @@ const InserterDraggableBlocks = ( {
blocks,
};

const blocksContainMedia =
blocks.filter(
( block ) =>
( block.name === 'core/image' ||
block.name === 'core/audio' ||
block.name === 'core/video' ) &&
ramonjd marked this conversation as resolved.
Show resolved Hide resolved
( block.attributes.url || block.attributes.src )
).length > 0;

const blockTypeIcon = useSelect(
( select ) => {
const { getBlockType } = select( blocksStore );
Expand Down Expand Up @@ -63,7 +72,7 @@ const InserterDraggableBlocks = ( {
? [ createBlock( 'core/block', { ref: pattern.id } ) ]
: blocks;
event.dataTransfer.setData(
'text/html',
blocksContainMedia ? 'media' : 'text/html',
serialize( parsedBlocks )
);
} }
Expand Down
7 changes: 5 additions & 2 deletions packages/block-editor/src/components/media-placeholder/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ export function MediaPlaceholder( {
}
}

async function onHTMLDrop( HTML ) {
async function onMediaDrop( HTML ) {
const blocks = pasteHandler( { HTML } );
return await handleBlocksDrop( blocks );
}
Copy link
Contributor

@talldan talldan Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );
}

Expand Down Expand Up @@ -380,7 +380,10 @@ export function MediaPlaceholder( {
}

return (
<DropZone onFilesDrop={ onFilesUpload } onHTMLDrop={ onHTMLDrop } />
<DropZone
onFilesDrop={ onFilesUpload }
onMediaDrop={ onMediaDrop }
/>
);
};

Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/drop-zone/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export function DropZoneComponent( {
label,
onFilesDrop,
onHTMLDrop,
onMediaDrop,
onDrop,
...restProps
}: WordPressComponentProps< DropZoneProps, 'div', false > ) {
Expand All @@ -60,13 +61,16 @@ export function DropZoneComponent( {
? getFilesFromDataTransfer( event.dataTransfer )
: [];
const html = event.dataTransfer?.getData( 'text/html' );
const media = event.dataTransfer?.getData( 'media' );

/**
* From Windows Chrome 96, the `event.dataTransfer` returns both file object and HTML.
* The order of the checks is important to recognize the HTML drop.
*/
if ( html && onHTMLDrop ) {
onHTMLDrop( html );
} else if ( media && onMediaDrop ) {
onMediaDrop( media );
} else if ( files.length && onFilesDrop ) {
onFilesDrop( files );
} else if ( onDrop ) {
Expand All @@ -84,6 +88,8 @@ export function DropZoneComponent( {
*/
if ( event.dataTransfer?.types.includes( 'text/html' ) ) {
_type = 'html';
} else if ( event.dataTransfer?.types.includes( 'media' ) ) {
_type = 'media';
} else if (
// Check for the types because sometimes the files themselves
// are only available on drop.
Expand Down Expand Up @@ -116,6 +122,7 @@ export function DropZoneComponent( {
( isDraggingOverDocument || isDraggingOverElement ) &&
( ( type === 'file' && onFilesDrop ) ||
( type === 'html' && onHTMLDrop ) ||
( type === 'media' && onMediaDrop ) ||
( type === 'default' && onDrop ) ),
'is-dragging-over-document': isDraggingOverDocument,
'is-dragging-over-element': isDraggingOverElement,
Expand Down
7 changes: 6 additions & 1 deletion packages/components/src/drop-zone/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export type DropType = 'file' | 'html' | 'default';
export type DropType = 'file' | 'html' | 'media' | 'default';

export type DropZoneProps = {
/**
Expand Down Expand Up @@ -26,4 +26,9 @@ export type DropZoneProps = {
* It receives the HTML being dropped as an argument.
*/
onHTMLDrop?: ( html: string ) => void;
/**
* The function is called when dropping media into the `DropZone`.
* It receives the media being dropped as an argument.
*/
onMediaDrop?: ( media: string ) => void;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm do you mean leveraging onDrop for the media type instead of adding a custom handler? I can give it a go!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

2 changes: 1 addition & 1 deletion test/e2e/specs/editor/blocks/image.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,7 +545,7 @@ test.describe( 'Image', () => {
dummy.style.left = 0;
dummy.draggable = 'true';
dummy.addEventListener( 'dragstart', ( event ) => {
event.dataTransfer.setData( 'text/html', _html );
event.dataTransfer.setData( 'media', _html );
setTimeout( () => {
dummy.remove();
}, 0 );
Expand Down
Loading