-
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
Drag and drop: Allow dragging from inserter or desktop to template parts #58589
Drag and drop: Allow dragging from inserter or desktop to template parts #58589
Conversation
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 Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf 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. |
Size Change: -9.7 kB (-1%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
@@ -219,7 +219,7 @@ export function useInnerBlocksProps( props = {}, options = {} ) { | |||
! isBlockSelected( clientId ) && | |||
! hasSelectedInnerBlock( clientId, true ) && | |||
enableClickThrough && | |||
! isDraggingBlocks(), |
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.
Is it still necessary to have isDraggingBlocks
given the addition of isDragging
? When would one use one and not the other? Where do we still use isDraggingBlocks
?
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 doesn't seem to be used much but it's a public API so we can't really get rid of it altogether 😅
We could maybe refactor existing uses of it but that's better done as a separate PR I reckon. Not sure if there is a case for knowing that the thing being dragged is specifically a block 🤔
} | ||
|
||
return state; | ||
} |
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 our unit test coverage for selectors, actions, and reducers is very high so should probably maintain that by adding unit tests for these new ones. Even though they're painfully simple... maybe ChatGPT can write tests for you.
Works well. Very straightforward, I like it. I'm just curious about Other than that, 👍 |
Co-authored-by: Robert Anderson <robert@noisysocks.com>
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 works very well in testing! Code change is pretty minimal and given that all sorts of non-block things can be dragged and dropped, it's a useful selector to have.
Thanks for the reviews, folks! I'll add in some tests as @noisysocks suggested and then look at merging this in. While in most cases I imagine we could probably move toward the new For now, the new action and selector is private so we've got a bit of wriggle room to tease apart how the differences may or may not be useful. |
Flaky tests detected in d8a3894. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7777615172
|
Testing this out and I noticed some oddities (using WP Playground to explore this PR) when trying to drag & drop the site logo in: drag.site.logo.movMaybe it's just playground but wanted to note just in case. |
Thanks @annezazu! Yep, that looks like the issue of Playground's added UI interfering with the positioning of the drop indicator line, as it pushes the whole editor canvas down in a way that Gutenberg isn't aware of. I'll write up a quick issue for it — I'm wondering if the Gutenberg logic can be a bit more resilient rather than expect that the editor UI isn't wrapped in another iframe. |
Opened a follow-up issue for the drop indicator line positioning over in #58756. I'll take a look at what might be causing it. |
What?
Follows on from #58423 and fixes #56182
Allow dragging from the block inserter into template parts, and allow dragging from the desktop to template parts.
Why?
As flagged by @annezazu in testing (see: #56182 (comment)), when dragging from the block inserter (or from the desktop) the behaviour to allow dragging into template parts isn't in place — the overlay still prevents the dragging from happening. It turns out, there are places where
isDraggingBlocks
is insufficient to detect whether the user is dragging. In both cases (dragging from the inserter or from the desktop) there is no "block" being dragged as it hasn't been created yet.How?
Add some additional state in the block editor to track when dragging starts and when it ends. This is a generic boolean, rather than storing any data about blocks being dragged.
When starting to drag from the inserter, and in the throttled drag handler in the editor's drop zones, we set this state to
true
. When the drag ends, we set it tofalse
.Then, use this state to skip adding the block overlay while a user is dragging. The result should be that we can now drag into the template part, but if a user goes to click on a block within the template part, as on trunk, they'll first select the template part, before clicking again to select further within it.
Note: This PR does add additional state to the block editor store — unfortunately due to how dragging from the desktop works and that mouse events are not fired when dragging over the window, I couldn't work out a way to implement this without the additional state. Hopefully, it's fairly simple state, though 🤞
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Dragging from the inserter
2024-02-02.15.11.13.mp4
Dragging from the desktop
2024-02-02.15.13.08.mp4