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

Add drop-to-upload support for image block #1888

Merged
merged 7 commits into from
Jul 14, 2017
Merged

Add drop-to-upload support for image block #1888

merged 7 commits into from
Jul 14, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 13, 2017

Closes #1701

This pull request seeks to enhance the image block to allow image upload by drag-and-drop.

Drag-and-drop

Implementation notes:

Included is a new DropZone component which is largely a ported copy of the one included in the Calypso project.

An initial attempt had focused on adding the drop zone to the entire editor canvas, but upon observing proposal at #1563 (comment) to use insertion point, the branch was later changed to focus on the image block. Some refactoring of notices and block removal lingers after this change in direction.

The changes modify the Dashicon component directly, which is not recommended since the file is generated from a separate repository. See follow-up task for updating Dashicon generated behavior.

Testing instructions:

Verify that after inserting a new image block, you can drag-and-drop an image from your computer onto the image placeholder to assign the block to use that image.

Follow-up Tasks:

  • Better error handling for failed uploads
  • More accurate file type supports (either by server-side localization, or REST API enhancements to expose this information)
  • Block post saving until upload completes
  • Update Dashicon component to support size prop

@aduth aduth added the [Feature] Blocks Overall functionality of blocks label Jul 13, 2017
event.preventDefault();
}

isWithinZoneBounds( x, y ) {
Copy link
Member

Choose a reason for hiding this comment

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

If a block is selected with a drop zone, and media is dragged over the editor, can we assume it's for this block? It may make the movement a bit easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

If a block is selected with a drop zone, and media is dragged over the editor, can we assume it's for this block?

Should we assume this? Even if I have a block selected, if there are two image blocks on the page, seems I should be able to drop an image on either, regardless of which is currently selected.

Copy link
Member

Choose a reason for hiding this comment

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

Also if we add the ability to drag image to create a new block at insertion point it'll get confusing.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I guess I kind of see it as paste where there is only one possible insertion point.

}
}

preventDefault( event ) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be a method of this class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this need to be a method of this class?

Yes, only so that we have a consistent reference to unbind in componentWillUnmount.

@@ -106,6 +111,39 @@ registerBlockType( 'core/image', {
icon="format-image"
label={ __( 'Image' ) }
className={ className }>
<DropZone
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to make an "ImageDropZone" component to abstract this away?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it make sense to make an "ImageDropZone" component to abstract this away?

Maybe. Prior to 52c848c there'd been a MediaDropZone component for inserting new image blocks. In this case, the behavior sets attributes from within the image block. To make this generic we might need to expose a way to map properties of a media entity to attributes of the block.

In general I think the image block could do for some separating into smaller components, but within its own folder/scope, not general purpose components.

setAttributes( { url: null } );
} );
} }
/>
Copy link
Member

Choose a reason for hiding this comment

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

Note: we would also want a button to upload directly without opening the media library.

transition: 0.3s opacity, 0.3s background-color;
}

&.is-dragging-over-element {
Copy link
Member

Choose a reason for hiding this comment

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

I am seeing the area highlighted even when dragging outside of the image block.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am seeing the area highlighted even when dragging outside of the image block.

Hmm, there's supposed to be two levels of highlight: One for when dragging over the page to indicate where drop zones are, then another when over the droppable area. Seems to not be working at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be fixed as of rebased 2fd845221d03a955e0dd7242c15dec0b9352074b

@mtias
Copy link
Member

mtias commented Jul 14, 2017

This looks good to me, outside of the test errors.

aduth added 7 commits July 14, 2017 14:17
Will need editor-wide drop behavior (#1563), but current proposal is insertion point. With existing DropZone implementation, easier to refactor as implementing on image block
@mtias
Copy link
Member

mtias commented Jul 14, 2017

Thanks for working on this one. Looks great.

@mtias mtias merged commit 2f93953 into master Jul 14, 2017
@mtias mtias deleted the add/drop-zone branch July 14, 2017 18:26
@jasmussen
Copy link
Contributor

Did this PR change the dashicon component here, but not upstream? I'd like to add a columns icon, but seeing a ton of red in the diff, where I'd expect only green for the new icon.

Here's the icon I'm trying to add: WordPress/dashicons#220

jasmussen added a commit that referenced this pull request Aug 1, 2017
Don't merge this. There's a weird amount of red in this diff. I'm pushing as a branch so we can discuss how it regressed in the upstream dashicons repo, possibly as part of #1888.
@jasmussen jasmussen mentioned this pull request Aug 1, 2017
@aduth
Copy link
Member Author

aduth commented Aug 1, 2017

@jasmussen I did update the component directly, but I also submitted an upstream pull request at WordPress/dashicons#219 which should have reconciled the differences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants