-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 dropping within template parts #58423
Drag and drop: Allow dropping within template parts #58423
Conversation
Size Change: -2 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
Do they need to know this technical detail, or is it enough to know that the block is in the position they chose?
|
That's exactly the kind of feedback I was hoping for, thanks Carolina! If it isn't a blocker given the points you describe, then that makes this PR easier to progress. I'll continue testing the changes here and see if we can get this ready for a proper review 👍 |
d7bd92f
to
d2982c6
Compare
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 is working really well in testing!
Only one slight weirdness: pressing undo after dragging a block from somewhere else in the canvas into a template part removes the block but doesn't re-add it in its previous location. It takes a second press of undo to get the block back.
The issue isn't caused by this change, because I can reproduce it in trunk by moving a block into a template part with the "Move to" keyboard interaction: undoing in that case also takes two presses. It's going to be more noticeable once this PR is merged though, so might be worth looking into fixing it soon (separately from this PR of course!)
Oh, and here's a video 😄 drag-into-template.mov |
Flaky tests detected in d2982c6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7720660102
|
Thanks for testing @tellthemachines!
Great catch — yes, let's look at the undo behaviour in a follow-up, assuming this PR lands. I've just pushed another small update: dragging and dropping within the content area in template preview mode should now be working correctly, too. I think this PR is ready for review now. I'll just ping @WordPress/gutenberg-design for visibility here, too, in case there are any concerns about the increased ability to drag. |
I've just smoked tested so far. Will spend more time with it tomorrow, but I'd say "no" that question. 😄 With careful mouse-driving, I'm able to place things where I intend. Working very nicely I'd say.
I can't only think of some sort of container-based highlighting, but that could get tricky I suppose moving up the DOM/Block tree? |
I don't think there are any concerns with the proposed behavior, though I agree the undo situation isn't optimal. My only comment is that the interaction becomes void if at some point we make it so that template parts can only be edited in focus mode, which is something I've seen proposed a few times. |
Let's double check and refine the visuals for the "can't drop here" chip from #58423 (comment) — I can't look today, but mainly noting. |
That is an existing design on trunk, so possibly worth a dedicated ticket to explore options? |
Thanks folks!
Yes, it'd be good to look into design updates separately. Okay, I think there's no objections to the proposed behaviour here for now, so I believe this PR should be ready for a final review now! And separately we can look into fixing the undo issue, too 👍 |
Just testing this again and wondering: should it be possible to drag template parts into other template parts? 2024-02-01.10.59.01.mp4I'm not sure it matters as it's consistent with block dragging behaviour, and I personally like being able to drag stuff wherever I like. |
Good question!
I'm leaning toward this, too. This PR is currently fairly simple, but I imagine we'd need to go through a few hoops to figure out if a drop zone is valid for determining whether to deny dragging a template part within another template part. We could always look into it in a follow-up if it looks like an issue for folks, possibly by extending logic in isDropTargetValid? |
Given that Navigation blocks are essentially template parts and they're mainly used in Headers, I'd say yes! |
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.
Nice small code change with a really great usability impact!
Very good point 😆 Also LGTM! |
Thanks for the reviews, folks! 🙇 I'll merge this in and then take a look at the undo problem separately. As far as I can tell we wind up with two records in the undo stack due to updating entities separately here for the main template and the template part when dragging between them (via a call to moveBlocksToPosition)... I'll be curious to see if we can group those two actions into the one history entry — in any case, it's a different part of the code to what we're altering in this PR, so it should hopefully be a better contained thing to look at on its own 🤞 Thanks again! |
What?
Fixes #56182
Allow dragging blocks within template parts in the site editor. Also, ensure that dragging and dropping works correctly within the content area in
template-locked
mode (that is, template preview mode in the post editor, or while editing pages in the site editor).Why?
Currently it's quite difficult for folks to drag blocks within a template into the header or footer areas as the editor canvas prevents users from doing so. It would be good to allow folks to be able to do this, though it does present an interesting challenge — how do folks know, before they let go of the mouse cursor, whether they are dropping within a template part, or just outside it?
How?
hasOverlay
so that it only applies when not dragging blocksisDropZoneDisabled
so that it is not disabled if there is an overlay settemplate-locked
modeQuestions for reviewer(s)
Is it a deal breaker that we can't see before dropping whether we're going to be dropping before, after, or within a template part? If it is a deal breaker, how might we visualise that a user will be dropping within a template part?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before
Trying to drag within the Header template part drags to an adjacent position:
2024-01-30.16.14.21.mp4
After
The user can now drag within the Header template part:
2024-01-30.16.12.34.mp4