-
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
List View: Allow dragging outside the immediate area by passing down a dropZoneElement #50726
List View: Allow dragging outside the immediate area by passing down a dropZoneElement #50726
Conversation
Size Change: +22.8 kB (+2%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
…a dropZoneRef from outside the list view
9f0599e
to
d925f68
Compare
packages/block-editor/src/components/list-view/use-list-view-drop-zone.js
Outdated
Show resolved
Hide resolved
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.
Confirming what @jasmussen sees. I can't seem to trigger this change either when dragging past the last item in the list. Everything built and working correctly from what I can see?
Thanks folks! Sounds like I might have broken something in the latests commits, I'll dig into it 👍 |
Flaky tests detected in 4eb834d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5051597076
|
Thanks again for testing and flagging the issue @jasmussen and @apeatling — it turns out I was testing with the I've pushed a fix in 970217e, so it should be working now, but do let me know if you still run into any issues! Updated: I've added a couple of simple tests in 84ac583 to test that the drop zone is flagged correctly. |
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.
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 well. I'm unsure of some the specifics around how the useDropZone
hook works, I personally always find these things a little hard to understand though!
// If a custom dropZoneRef is passed, use that instead of the element. | ||
// This allows the dropzone to cover an expanded area, rather than | ||
// be restricted to the area of the ref returned by this hook. | ||
const element = dropZoneRef?.current ?? elem; |
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'm wondering how this will work in terms of the cleanup of the hook.
Because it's a ref effect, I think the callback will typically only trigger when elem
is unmounted, not when dropZoneRef.current
is unmounted.
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.
Good question! I think this is working pretty well so far. To confirm I logged out element
within the callback, and it appears to be the right element:
As far as I can tell the behaviour is something like the following:
- We pass in a ref to
useDropZone
, butuseDropZone
also returns aref
. Thatref
is still used in the list view, to attach to theTreeGrid
component. - When
TreeGrid
unmounts, that results in the cleanup callback being called. - Because the cleanup callback was instantiated with a reference to
element
being the one passed in bydropZoneRef
the cleanup callback cleans up the passed in element. - Also, because we have
dropZoneRef?.current
in the dependencies array ofuseRefEffect
when the passed in ref changes, the cleanup appears to correctly fire, too. From checking the docs, this appears to be consistent with how useEffect works in that the cleanup fires not only when the component unmounts, but whenever any of the dependencies change. The doc comment foruseRefEffect
appears to confirm this here, too:gutenberg/packages/compose/src/hooks/use-ref-effect/index.ts
Lines 11 to 29 in 62b3b0e
/** * Effect-like ref callback. Just like with `useEffect`, this allows you to * return a cleanup function to be run if the ref changes or one of the * dependencies changes. The ref is provided as an argument to the callback * functions. The main difference between this and `useEffect` is that * the `useEffect` callback is not called when the ref changes, but this is. * Pass the returned ref callback as the component's ref and merge multiple refs * with `useMergeRefs`. * * It's worth noting that if the dependencies array is empty, there's not * strictly a need to clean up event handlers for example, because the node is * to be removed. It *is* necessary if you add dependencies because the ref * callback will be called multiple times for the same node. * * @param callback Callback with ref as argument. * @param dependencies Dependencies of the callback. * * @return Ref callback. */
So, all up, I don't think there are any stray event listeners not being removed, as far as I can tell.
Do let me know if you think I've missed anything here, though!
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.
When TreeGrid unmounts, that results in the cleanup callback being called.
Sorry if I'm making this more complicated. Given this is being introduced as a generic stable API, I'm mainly thinking that there could be different ways it's used (or misused):
- The
dropZoneRef
could reference a completely separate part of the DOM that isn't mounted/unmounted at the same time as the effect's component. - The
dropZoneRef
could reference a parent of the effect's component that isn't mounted/unmounted at the same time.
I don't know if there are tests that can be added to assert these different cases work as expected.
It also might be worth expanding upon the docs for the prop to ensure that the right usage of this API is promoted. There's an example of this for Popover's anchor
prop that recommends particular usage - https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/popover/README.md#anchor-element--virtualelement--null.
Also, because we have dropZoneRef?.current in the dependencies array of useRefEffect when the passed in ref changes, the cleanup appears to correctly fire, too
This is the part that often trips me up. With dropZoneRef
being a ref type, it won't trigger a re-render when it changes. I've sometimes opted to store an element using useState
for that reason (which is also what Popover
recommends).
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.
Sorry if I'm making this more complicated.
No apology necessary! I'm happy to go as slowly as needed with this PR — I always like to be a bit more cautious when making changes to lower-level packages such as compose
, and it's important we get it right and don't introduce any issues 👍
I don't know if there are tests that can be added to assert these different cases work as expected.
Good example use cases, it's worth digging in, I'll do a little more exploration to see if it's possible to come up with additional test cases. I think expanding the test cases there is probably a good path toward building confidence around the API change.
It also might be worth expanding upon the docs for the prop to ensure that the right usage of this API is promoted. There's an example of this for Popover's anchor prop that recommends particular usage
Good idea, for some reason useDropzone
isn't documented in the compose
package's readme, but I'll have a go at writing an entry for it.
This is the part that often trips me up. With dropZoneRef being a ref type, it won't trigger a re-render when it changes. I've sometimes opted to store an element using useState for that reason (which is also what Popover recommends).
Ah, good point, because the ref reference itself isn't changing, only current
🤔. In practice, I haven't yet run into any issues in this PR so far. In terms of using useState
, do you mean we'd do that internally within useDropZone
, or are you imagining that the consumer would use useState
, and we'd pass the element directly to useDropZone
instead of a ref
?
Edit: I think you probably mean the latter — and I can see that the url popover for example, uses the state approach here:
gutenberg/packages/block-editor/src/components/url-popover/image-url-input-ui.js
Line 251 in 493eac0
ref={ setPopoverAnchor } |
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.
Edit: I think you probably mean the latter — and I can see that the url popover for example, uses the state approach here
Yep, exactly that 👍
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.
Update: I've done the following:
- Swapped out
dropZoneRef
for adropZoneElement
and updated usage to use state for storing the element before passing it along to theListView
(and then on touseDropZone
) - Added a README file for
useDropZone
— because the hook is still currently exported as experimental, I couldn't add directly to thecompose
package readme. I see a couple of other experimental hooks also include their own README files in thecompose
package, so this seemed like a decent place to store the documentation for now, while the hook is still "experimental". Happy for any feedback there, I found it a bit challenging coming up with straightforward wording 😅
I haven't looked into exploring adding other tests just yet, but will dig in tomorrow.
Thanks again for the feedback!
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.
because the hook is still currently exported as experimental, I couldn't add directly to the compose package readme.
Thanks, didn't realise it was experimental. It must have been that way for ages!
My hunch is that using state to store the element will probably solve the issues, hopefully that's the case.
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.
Thanks for the re-reviews! @talldan and @kevin940726 — unfortunately I haven't had much luck writing additional tests for this, as I can't seem to get the event handlers to fire within an individual test (I've tried a At this stage, do you think adding more tests is a blocker to landing this PR? Happy to keep looking into it, but I'm currently a bit stumped as to how to reliably write tests for this hook where the drag events are fired 🤔 |
I don't really know a lot about the testing situation, so I'm not very useful in offering suggestions! I think it's good to merge given the docs now recommend using the hook in the same way List View uses it (and that's working well). |
Co-authored-by: Daniel Richards <[email protected]>
Thanks Dan! I've just committed your Readme suggestion, and once the GH actions pass, I'll merge this in. |
What?
Part of addressing #43881 and #32438, alternative to #49420 and #50689. Overall, this PR makes dragging to the top and bottom positions of the list view easier.
Allow dragging and dropping within the list view, beyond the bounds of the list view itself, when passed a
dropZoneElement
(an element to be used as the drop zone, typically a parent of the final drop zone ref). Within the list view sidebar, pass the list view container area as adropZoneElement
to theListView
component.Then, when the mouse leaves that drop zone area, clear the drop target so that the blue line no longer appears. The result should be that the blue drop indicator line within the list view is now accurate — it will only display when a drop will occur, and it displays across the whole list view area, making it easier to drag to the top and bottom-most positions.
Why?
One of the frustrations with drag and drop in the list view is that it's very difficult to drag to the top and bottom-most positions. This PR hopefully makes that experience feel more flexible and accurate.
How?
dropZoneElement
element touseDropZone
and use its element if available, falling back to the one passed down byuseDropZone
.ListView
anduseListViewDropZone
levels.ListView
.null
so that the drop indicator disappears.Testing Instructions
trunk
, it should be a lot easier to drop to these positions.Screenshots or screencast
Note how in the before version, the Heading looks like it can be dropped to the very bottom of the area by dragging well below, but it cannot. In the after version, the drop is respected correctly.
Some more details of the behaviour in this PR: