Allow dragging-and-dropping images from the inserter to image blocks #49673
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
|
Size Change: +228 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in ab7605cd5e9053a3213e05ed73112caf4b66633f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4796139894
|
kevin940726
force-pushed
the
add/image-drop-html
branch
2 times, most recently
from
a54f544
to
b0ae1b3
Compare
kevin940726
commented
Apr 10, 2023
| @@ -4,12 +4,6 @@ figure.wp-block-gallery { | |||
| // See https://github.com/WordPress/gutenberg/pull/10358 | |||
|
|
|||
| display: block; | |||
| &.has-nested-images { | |||
There was a problem hiding this comment.
This will have some unexpected side effects, unfortunately. Namely it will no longer be possible to drag between two image blocks when the gap is zero. We'll have to determine which is the better trade-off, and what potential other options we have to solve this.
kevin940726
added
[Feature] Inserter
ntsekouras
reviewed
Apr 12, 2023
kevin940726
force-pushed
the
add/image-drop-html
branch
from
2ed11b1
to
89365c4
Compare
kevin940726
commented
Apr 20, 2023
packages/block-editor/src/components/media-placeholder/index.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Apr 20, 2023
packages/block-editor/src/components/inserter/media-tab/media-preview.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/media-placeholder/index.js
Outdated
Show resolved
Hide resolved
kevin940726
force-pushed
the
add/image-drop-html
branch
2 times, most recently
from
773864c
to
6308224
Compare
youknowriad
reviewed
Apr 24, 2023
packages/block-editor/src/components/media-placeholder/index.js
Outdated
Show resolved
Hide resolved
kevin940726
requested review from
ajitbohra,
ellatrix and
geriux
as code owners
kevin940726
force-pushed
the
add/image-drop-html
branch
from
d1032c4
to
ab7605c
Compare
|
I updated it to also handle audio and video blocks, and ignore empty blocks. This is now ready for review! |
|
Am I doing something wrong, I can drag and drop an image into the placeholder, nothing happens for me. The first time it worked but stopped working since. |
kevin940726
force-pushed
the
add/image-drop-html
branch
from
2e4f9ab
to
de7ade2
Compare
|
@youknowriad Oh oops! I pushed the wrong commit. I believe it's now fixed! |
|
Ok this works better :) but there's a few notes that I think we should look at:
The biggest question for me is the Gallery block. It seems this PR has a big impact on how drag and drop works there. In trunk, I'm able to drag and drop to reorder images easily in the Gallery image without being bothered by placeholders for the image blocks within the gallery. I can also drag and drop an image from the inserter into the gallery block, it adds the image to the gallery. These flows don't work as smoothly in this branch. So the question here is whether we want maybe to disable that drop zone within gallery blocks or within image blocks that are "filled" (keep it only in the placeholder state). Also, let's get some design eyes here. cc @jasmussen @jameskoster |
kevin940726
force-pushed
the
add/image-drop-html
branch
from
c3ccbed
to
e2eddff
Compare
kevin940726
commented
May 3, 2023
| @@ -4,12 +4,16 @@ | |||
|
|
|||
| - `CheckboxControl`, `CustomGradientPicker`, `FormToggle`, : Refactor and correct the focus style for consistency ([#50127](https://github.com/WordPress/gutenberg/pull/50127)). | |||
|
|
|||
| ### Breaking Changes | |||
|
|
|||
| - `onDragStart` in `<Draggable>` is now a synchronous function to allow setting additional data for `event.dataTransfer` ([#49673](https://github.com/WordPress/gutenberg/pull/49673)). | |||
There was a problem hiding this comment.
Unfortunately, this is a breaking change, but I think it actually fixes a bug so it might not be that impactful. c.c. @ciampo if you think this is acceptable :).
There was a problem hiding this comment.
Sorry, still catching up through the notifications pile. Sounds good to me!
There was a problem hiding this comment.
We should probably add a dev note for this change, though. I've added the GH label
| isDragging.current = true; | ||
| // Defer hiding the dragged source element to the next | ||
| // frame to enable dragging. | ||
| window.requestAnimationFrame( () => { |
There was a problem hiding this comment.
This is to retain the former behavior of asynchronous onDragStart before. I also changed setTimeout to requestAnimationFrame to better describe the intent.
Kapture.2023-05-03.at.15.31.38.mp4I believe this now matches more closely to what we want now. We eventually will still want to handle blocks drop in some other components and will need the multiple data types trick, but fortunately not for the c.c. @jasmussen @jameskoster @youknowriad in case I missed anything 🙇 . |
|
Can you still drop an image from your local files onto the canvas to replace? If so this looks excellent! |
|
@jameskoster Yep, that's always supported! |
|
That looks like it threads the needle. Nice work. Let's just get the code sanity checked but otherwise landed. I'm noticing that there's a horizontal scrollbar when you drag an OV image with a caption into the gallery. I'm assuming this is an existing issue in trunk. If yes, let's create an issue for that and put it in the polish board. |
youknowriad
reviewed
May 3, 2023
|
Thanks for this one! |
ciampo
added
the
Needs Dev Note
24 tasks
6.3 Dev NoteThe This change should have minimal impact on existing code. However, if it does affect your code, you can maintain the original behavior by wrapping your event callback inside a -onDragStart={ () => { doSomething(); } }
+onDragStart={ () => { setTimeout( doSomething, 0 ); } } |
kevin940726
added
has dev note
This was referenced Jul 11, 2023
6 tasks
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?
Alternative to #49297.
Allow dragging-and-dropping images from the inserter to image blocks.
Why?
Close #48919. See the issue for more info.
How?
I added some inline review comments below for some implementation details. The main idea is derived from #49297.
Testing Instructions
Testing Instructions for Keyboard
This is a feature about drag-and-drop, so unfortunately it cannot be tested with keyboard (yet).
Screenshots or screencast
Kapture.2023-04-10.at.16.31.57.mp4