Drag and drop: allow dragging to the beginning and end of a document #56070
Conversation
andrewserong
added
[Type] Enhancement
|
Size Change: +429 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
andrewserong
force-pushed
the
try/allow-drag-to-top-and-bottom-of-editor-canvas
branch
from
27de1fd
to
44c1324
Compare
andrewserong
force-pushed
the
try/allow-drag-to-top-and-bottom-of-editor-canvas
branch
from
44c1324
to
11376a9
Compare
andrewserong
changed the title
andrewserong
requested review from
talldan,
apeatling,
ramonjd and
tellthemachines
|
This is ready for review now. I'll just do a wider ping to @WordPress/gutenberg-core for visibility as this PR proposes adding |
andrewserong
changed the title
There was a problem hiding this comment.
This is working really well for me! Tested in post editor with both classic and block theme, and in the site editor on a couple different-sized templates. There's less room to manoeuvre at the top of the site editor so it doesn't make much difference there, but the improvement is noticeable at the bottom on shorter templates.
The code change is very clean and minimal too!
| isToBeIframed | ||
| ? ref.current?.parentNode | ||
| : ref.current | ||
| } |
There was a problem hiding this comment.
Interesting, I'd have expected this to be contentRef but I guess it works because contentRef includes ref? I'm assuming that what matters is a shared ref between BlockCanvas and dropZoneElement, right?
There was a problem hiding this comment.
Yep! Yeah, interestingly enough it's contentRef in the site editor because that's the name of the direct ref, whereas in the post editor contentRef is the merged ref. So, ref contains the .current reference to the node. From a little digging (I think this is how it works) in the post editor contentRef is what gets passed to BlockCanvas as it contains a callback function that will update the list of refs to be merged, but we need to use ref.current to grab a reference to the element itself. If we attempt to access contentRef directly (or pass it anywhere else), we just get the callback function rather than the element, as that's what's returned by useMergeRefs.
There was a problem hiding this comment.
Oooh I see, thanks!
|
Thanks for the quick review @tellthemachines! I'll let this PR sit overnight just in case anyone else has any feedback, and if not, will merge this in tomorrow 🙂 |
|
Correct my understanding: we want to change the drop zone element for the top level drop zone right? I don't see the new API used for the container blocks themselves? (Maybe I'm wrong, I'm just trying to make sense of the code) Is the goal to make the whole "canvas" a dropzone? Can we achieve that differently maybe? In my mind, after the recent component changes, it's the The other question is, do we need this useInnerBlocks option? Is this an API that we expect container blocks will use as well or it's only for the canvas, in which case we could find ways to make it private or define a different drop zone in that component. |
|
Btw, don't consider that blocking, trying to ask the questions to make sure we're implementing the right thing. |
|
Is the |
Great questions, thanks for the input @youknowriad!
Yes, I think you're following this PR correctly. The goal is for the whole canvas to be treated as a dropzone so that a user can drag anywhere within the canvas and the top and bottom-most positions will be treated as drag-to-top and drag-to-bottom, similar to how the list view works now. I like the idea of this happening automatically somehow via For now, since we don't have anything like that hooked up for Thanks for flagging them @ramonjd — I don't think this PR will help for them as the feature here just allows folks to specify a container for the overall drop zone area, rather than addressing where folks drop within that overall area. I think this PR is in a good place now that we're using Happy to help look into alternatives in follow-ups, of course! |
|
Flaky tests detected in c6d84ac. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6898289799
|
andrewserong
deleted the
try/allow-drag-to-top-and-bottom-of-editor-canvas
branch
| const { | ||
| __unstableDisableLayoutClassNames, | ||
| __unstableDisableDropZone, | ||
| __unstableDropZoneElement, |
There was a problem hiding this comment.
If we want to make this private, prefixing is not the way anymore. I think we're now avoiding any new __unstable or __experimental prefix.
There was a problem hiding this comment.
I just read your message above, you should use the lock/unlock API to mark this private. These days prefixing or not doesn't impact our policies for new APIs.
There was a problem hiding this comment.
Thanks for the suggestion and extra context @youknowriad! I've had a go at doing this over in #56308, would love your feedback to see if that's what you had in mind. Due to how useInnerBlocksProps is written and used, the solution wound up being a little indirect.
A potential alternative would be for us to stabilise dropZoneElement for useInnerBlocksProps and BlockList if we're comfortable with it as an API. We could then still look into further follow-ups that abstract it away a little further for the post and site editors (or folks building custom block editors).
There was a problem hiding this comment.
If we're making this private with the intention of removing it later and making the drop zone part of BlockCanvas as suggested above, my main question as to whether we should go one way or the other is if it would be useful at all to be able to set a drop zone in inner blocks. #26049 has been open for 3 years with no comments and few links to it, and the issue itself only links to one potential use case, so there doesn't seem to be a huge demand for such an API.
There was a problem hiding this comment.
#26049 has been open for 3 years with no comments and few links to it, and the issue itself only links to one potential use case, so there doesn't seem to be a huge demand for such an API.
I suppose the presence of that issue nudges me slightly toward "let's go for private for now" as it means we can then continue to explore the idea of allowing it for (likely 3rd party) container blocks in follow-up PRs (by digging into the potential use case). If we find it isn't useful, we can abandon it and go with making it part of BlockCanvas, and if it is useful, we can make the API public (and also potentially still explore making it an internal implementation detail of BlockCanvas). In both cases, it sounds like the private approach in #56308 could make for a decent interim place for now? I agree it doesn't sound like there's much demand for it for core container blocks to set their own element 🤔
I don't feel strongly about which way we go, though, my main interest is getting us to an interim state we feel comfortable with for now 🙂
There was a problem hiding this comment.
#26049 has been open for 3 years with no comments and few links to it, and the issue itself only links to one potential use case, so there doesn't seem to be a huge demand for such an API.
A lot of folks complain generally about the drag and drop experience being clunky without really providing specifics or knowing the underlying reason. I don't think I did a good job of writing that issue, I should really have led with the problem rather than a solution, and I think that has meant not many comments.
and the issue itself only links to one potential use case
There are a few blocks it happens with - Cover, Media & Text and Group. It happens any time there's padding around the inner blocks element.
There was a problem hiding this comment.
It happens any time there's padding around the inner blocks element.
Ah, excellent. Thanks for confirming, Dan! That gives us something concrete to investigate here 👍
There was a problem hiding this comment.
I can reproduce with the Cover block. I've added a screengrab to #26049 (comment) to demonstrate the problem. I'm happy to pick up that task next as it's very related to this PR.
There was a problem hiding this comment.
Another update: given that solutions to #26049 will likely involve the dropZoneElement prop on useInnerBlocksProps, I've opened up another PR to stabilise the dropZoneElement prop, over in #56313, as an alternative to making it private. We can likely go either way, but stabilising is the simpler code change.
What?
Possible fix for: #32438 and #55474, implements a potential solution for #26049
Allow dragging to the top and bottom of the editor canvas, to drag and drop more easily to the beginning or end of the document. This uses a similar approach to that used in the list view in #50726
Why?
Currently it's quite fiddly to drag to the beginning and end of the document. One way to resolve this is to pass in a ref to a larger around within which we allow dragging and dropping.
How?
useInnerBlocksPropsallow passing in an element to be used as the drop zone area, and use that to allow drag and drop to the top or bottom of the document. This builds on the solution earlier used for the list view in List View: Allow dragging outside the immediate area by passing down a dropZoneElement #50726.htmlelement) so that the full area of the iframe is used for the dropzone. This allows drag and drop over the very top area of the post editor (above the post title) to be considered as intending to drag to the beginning of the document, and the very bottom area of the site editor when a short template is used to flag dragging to the end of the document. In other words, if thebodyelement is shorter than the full area of the iframe, the full area of the iframe is still used to determine whether a drop target should be considered.To-do
Testing Instructions
trunkyou can only drag to the last block within the templateScreenshots or screencast
Before
2023-11-16.12.02.27.mp4
After
2023-11-16.12.06.13.mp4