Hide drop indicator where block isn't allowed to drop. #56843
Conversation
|
Size Change: +570 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 240cd39. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7284552776
|
tellthemachines
added
[Type] Bug
tellthemachines
requested review from
SaxonF,
jasmussen,
apeatling,
richtabor,
ramonjd and
andrewserong
tellthemachines
force-pushed
the
fix/drop-indicator-not-allowed
branch
from
7acdd54
to
43a1bc0
Compare
There was a problem hiding this comment.
Nice work, it feels much better not showing a drop indicator when you can't drop somewhere to me, and I also don't mind the opacity to flag that a drop isn't available. It's really nice seeing some visual feedback between a valid area and a not valid area 👍
Some of my review comments might be a bit out of order, I'm sorry, so I'll try to line up some of my thinking as I was looking through the code change:
Instead of toggling disabling the drop zone altogether, would it be possible to call hideInsertionPoint instead, from within the throttled useCallback in useBlockDropZone (or update the showInsertionPoint call to pass in null)? I haven't used that before, but from looking at the BlockDropZonePopover component, it looks like it checks that state here, so I was wondering if that might be a simpler way to hide the drop indicator? (I'm assuming that when it's hidden in state, that results in the drop indicator disappearing altogether, but I haven't tested it)
Then, potentially BlockDraggable could also use getBlockInsertionPoint to see if it's nullish? That might also help with syncing between the two bits of code, as the opacity change would only happen when the useCallback fires the changes in useBlockDropZone 🤔
I'm not sure if all that would work, there's quite likely a bunch of complexity there that I'm not aware of, so apologies if that's not a helpful idea, or if it's something you already tried while exploring this!
| * @param {string} targetClientId The client id of the target. | ||
| * @return {boolean} Whether the dragged blocks can be dropped on the target. | ||
| */ | ||
| export function useIsDropTargetValid( draggedBlockClientIds, targetClientId ) { |
There was a problem hiding this comment.
This hook appears to get called even when a drag event isn't happening. Is it possible to move some of the logic to the throttled useCallback so that we only figure out the allowed blocks in the throttled function that gets called while a user is dragging? Ideally it'd be good to limit what gets called before the drag actions occur, but it might not be possible to move everything over 🤔
There was a problem hiding this comment.
This could be refactored to a simple function taking getBlockType, allowedBlocks, draggedBlockNames, targetBlockName as parameters and then called inside the throttle. I'm unsure if determining isBlockDroppingAllowed inside the throttle will cause any lag but let's give it a try 😄
| count={ clientIds.length } | ||
| icon={ icon } | ||
| clientIds={ clientIds } | ||
| targetClientId={ targetClientId } |
There was a problem hiding this comment.
Is this addition currently doing anything? It looks like BlockDraggableChip isn't currently using it?
There was a problem hiding this comment.
yeah I must have left that in while experimenting 😄
|
|
||
| const [ targetClientId, setTargetClientId ] = useState( null ); | ||
|
|
||
| const isDropTargetValid = useIsDropTargetValid( clientIds, targetClientId ); |
There was a problem hiding this comment.
Does the BlockDraggable component need to know whether the drop target is valid? Could we use the getBlockInsertionPoint state to figure out if an insertion point is currently set instead?
There was a problem hiding this comment.
I don't think so; given that getBlockInsertionPoint is always meant to return an index, it would be hard to work out which targets are valid.
There was a problem hiding this comment.
Ah, gotcha! Looks like there's another selector isBlockInsertionPointVisible which handles the null case. Could that work if we'd called hideInsertionPoint in the main throttled useCallback in useBlockDropZone?
There was a problem hiding this comment.
Oh nice find! Lemme try that out
There was a problem hiding this comment.
Ok checking if the insertion point is visible does fix the discrepancy between transparent drag chip and inserter, but I found it necessary to keep the check for valid insertion point in place, because otherwise the chip keeps flickering every time the insertion point switches places. Still it's working much better now so thanks for the tip!
| // and if it's a container block | ||
| if ( | ||
| targetClientId !== newTargetClientId && | ||
| getBlockListSettings( newTargetClientId ) |
There was a problem hiding this comment.
Is getBlockListSettings( newTargetClientId ) checking for nested blocks to see if it's a container?
I think calling a select outside useSelect is an anti-pattern as it risks not returning fresh data when the store is updated.
I tested by updating the list settings for a block via wp.data.dispatch('core/block-editor').updateBlockListSettings() and it looks like the return state is updated, I guess because a rerender is triggered by another prop. So it's working in that case as far as I can tell.
cc @jsnajdr in case I missed something.
There was a problem hiding this comment.
I don't think it's an anti-pattern, we use select functions outside of useSelect all over the place 😅
getBlockListSettings was the most reliable way I found of checking whether a block is a container, given that only containers are block lists (and they might not have any inner blocks if they're empty). I'm definitely open to ideas on this one though 😄
There was a problem hiding this comment.
Actually scratch that, I don't think we need to check for the container anymore 😂 now we're using the inserter visibility as per @andrewserong's suggestion.
Still keen to hear about preferred code patterns though 👀
There was a problem hiding this comment.
Here getBlockListSettings is called in an event handler (onDragOver) and that's fine. We want to read the fresh value when the event is being dispatched, and that's what happens here. No rerenders need to be triggered when the value changes.
The only thing I would change here is to write the code, for clarity, as two separate useSelect calls:
const { getBlockListSettings } = useSelect( blockEditorStore );
// or alternatively
const { getBlockListSettings } = useRegistry().select( blockEditorStore );
const { ...rest } = useSelect( ( select ) => ... );One of them just gets a reference to the selector, without any store subscriptions or reactivity, and the second reads data in the usual reactive way.
There was a problem hiding this comment.
Thanks for confirming and setting me straight on that one, @jsnajdr!
And sorry about the misdirection @tellthemachines
There was a problem hiding this comment.
Ok thanks for the clarification!
There was a problem hiding this comment.
The only thing I would change here is to write the code, for clarity, as two separate useSelect calls
There are other selectors we're grabbing directly and using inside the effect such as getBlockType and getAllowedBlocks. I guess following that logic they should all be in a separate useSelect call?
|
This a sweet improvement. 🎉 I can drop things anywhere in trunk! 2023-12-13.15.25.28.mp4This PR has more sensible feedback: 2023-12-13.15.08.43.mp4I just pinged about a possible risk of calling the |
|
Thanks for the feedback and testing folks! I think all the code suggestions have been addressed so far. The experience is much improved but I'm now seeing a slight flakiness when moving over large non-container blocks such as Image. I think that might be due to the removal of the check for container status with |
There was a problem hiding this comment.
This is testing pretty well for me! I did notice a bit of inconsistency with when the opacity styles are applied when dragging from the list view. Sometimes the drag chip was greyed out, and sometimes it wasn't. A subtle bit was when dragging between Image blocks in a Gallery block where it flickers a little:
2023-12-14.10.25.02.mp4
The main issue I ran into is a performance issue while dragging over blocks — I think it's the onDragOver event in BlockDraggable, which will likely need to be throttled? (Or alternately removed, and consolidate the show/hide in useBlockDropZone's callback)
2023-12-14.10.31.45.mp4
Another potential option, if it proves to be tricky to resolve the flickering with the opacity issue, could be to split this PR into two: one PR to implement the hiding the drop indicator line, and then a separate follow-up PR to look at the opacity change for the drag chip?
| <div className="block-editor-block-draggable-chip-wrapper"> | ||
| <div | ||
| className="block-editor-block-draggable-chip-wrapper" | ||
| style={ { opacity: 'var(--wp--block-draggable-valid-opacity)' } } |
There was a problem hiding this comment.
Why is this applied via an inline style and CSS variable instead of by classname and direct value in style.scss? Or to put it slightly differently, does the JS version of the component need to have an opinion about how it's styled?
There was a problem hiding this comment.
Good question! I don't think it has to be applied as an inline style; I initially added it that way for the sake of development speed. It does have to be a CSS custom property, because its value needs to change when the body class gets added, and given that the chip we see on the page is a stored copy of itself changing classes on the chip itself won't work. Essentially the custom property is acting like a ref to the element, so we can manipulate its value from outside.
There was a problem hiding this comment.
It does have to be a CSS custom property, because its value needs to change when the body class gets added
Oh, I was imagining something like the following:
.block-editor-block-draggable-chip {
opacity: 1;
}
.block-draggable-invalid-drag-token {
.block-editor-block-draggable-chip {
opacity: 0.6;
}
}
Or does that not work? (Totally fine to go with a CSS variable by the way, just trying to understand what the limitations are 🙂)
There was a problem hiding this comment.
Oh right, yeah that should work too! Would that be preferable to a custom property? I don't think specificity matters either way.
There was a problem hiding this comment.
I don't feel strongly either way, but I'd lean slightly more toward the nested selectors so that it's easy to swap out for (or combine) other styles like background color, border, or other subtle styling changes that folks might want to explore in the future.
There was a problem hiding this comment.
Oh good point, yeah that would make it easier to change styles altogether if we wanted to.
| return; | ||
| } | ||
|
|
||
| const onDragOver = ( event ) => { |
There was a problem hiding this comment.
I think this callback might need to be throttled similar to the code in useBlockDropZone as this currently fires very frequently:
2023-12-14.10.37.55.mp4
There was a problem hiding this comment.
Hrm yeah I'm wondering how best to do that given the inability to use hooks inside the effect callback 🤔
There was a problem hiding this comment.
I think I ran into this issue when testing, it seemed to crash in Safari and I couldn't move the chip any more.
There was a problem hiding this comment.
Oo wait I see there's a throttle util in compose that we may be able to leverage, will try that!
There was a problem hiding this comment.
Ok the throttle seems to make it better! I set it to the same as draggable.
There was a problem hiding this comment.
That's a little better, but it's still lagging a bit for me with the throttle at 16 (it's more visible when using CPU throttling in dev tools). I think a couple of things might be at play:
- Since the
useEffectdoesn't use a dependency array, it seems that it's being called quite frequently, and potentially adding (and removing) event listeners too frequently? For example, if I increase the throttle time to2000, it still appears to be firing rapidly - In
draggable,16msis needed because it's updating the position of the drag chip. Here, I'm wondering if we can use a bigger delay / throttle, if that could smooth over the flicker when dragging between image blocks in a gallery block? I.e. if we had it at200msto matchuseBlockDropZonethen if folks are dragging quickly between blocks, hopefully it shouldn't flicker as much?
Would it work to maybe move defining the onDragOver callback to be outside of the useEffect (possibly in a useCallback) so that the useEffect could have a dependency array and only attach the event listener once? Apologies if you've already tried that!
There was a problem hiding this comment.
So I had a think about this 😄 and realised that we actually don't need to use local state for targetClientId, and we can use a dependency array for the effect, which improves things a little, but it still has to re-render whenever either draggedBlockNames or visibleInserter change. Moving the callback out of the effect makes no difference, because it calls whichever version of the callback was attached to the event listener when the effect ran, so if we want the values inside the callback to change the effect has to re-run.
I also tried increasing the throttle time and it doesn't seem to make things any worse, so might leave it at 200 or so. I just pushed an update, let me know if it improves things at all!
There was a problem hiding this comment.
Nice work! That appears to have fixed up the performance issues for me. Subjectively, without CPU throttling, it's feeling smooth now, and with CPU throttling, it seems much the same as trunk to me now 🎉
| // Add a dragover event listener to the editor root to track the blocks being dragged over. | ||
| // The listener has to be inside the editor iframe otherwise the target isn't accessible. | ||
| // Check if the dragged blocks are allowed inside the target. If not, grey out the draggable. | ||
| useEffect( () => { |
There was a problem hiding this comment.
Is this useEffect and its onDragOver still needed? Since the useBlockDropZone is hiding the insertion point, could we replace the whole useEffect with something like:
useEffect( () => {
if ( ! visibleInserter ) {
window?.document?.body?.classList?.add(
'block-draggable-invalid-drag-token'
);
} else {
window?.document?.body?.classList?.remove(
'block-draggable-invalid-drag-token'
);
}
}, [ visibleInserter ] );
Then the logic of when to show and hide the insertion point could live in one place, so that the BlockDraggable doesn't have to also perform checks with its own drag event handler.
Or is there another reason why this component needs to perform its own separate checks?
There was a problem hiding this comment.
Yeah, so the crucial bit here is finding the targetClientId, which tells us the container that the draggable is currently hovering over. To find that we need a dragover event listener attached to the inside of the editor iframe (if it's on the outside, it won't be able to access the elements inside the iframe). We need the targetClientId to check if the drop target is valid, which is necessary because if the chip is hovering over a valid container but there's no drop indicator currently showing (if we're moving between drop areas for instance), we don't want it to grey out.
Unfortunately there's not much that can be done to restrict how many times this runs without slowing down the responsiveness of the drag chip to what it's hovering over. The problem here is that, unlike with the drop indicators that exist statically between all blocks so we know where they're placed from the start, we need to find out what the draggable is being dragged over, and afaik the only way to do that is through the event target of a dragover listener.
There was a problem hiding this comment.
We need the targetClientId to check if the drop target is valid, which is necessary because if the chip is hovering over a valid container but there's no drop indicator currently showing (if we're moving between drop areas for instance), we don't want it to grey out.
Ah, gotcha! Thanks for the explanation, that makes sense now 👍
Unfortunately there's not much that can be done to restrict how many times this runs without slowing down the responsiveness of the drag chip to what it's hovering over.
Yeah, it can be really tricky to get the right balance when slowing things down so that it's slow enough for performance reasons, but fast enough to feel responsive to a user! 😅
we need to find out what the draggable is being dragged over, and afaik the only way to do that is through the event target of a dragover listener.
That sounds about right to me, too. If it winds up being a bit of a rabbithole trying to get it working smoothly, would it be worth splitting out the useBlockDropZone change and landing that first? Totally cool if you want to keep it all together while figuring everything out of course!
|
Thanks for the further testing and feedback!
Yeah I think this is the same issue I spotted yesterday evening and commented on here. |
I like this idea too. I'd say combined with the likes of #56843 (comment) would communicate strongly. |
|
Thanks for the suggestions folks! There doesn't seem to be a forbidden icon exactly like the one @apeatling suggests in our icon library so I've used our cancel one for now. I'm not sure it works as well but we can use it to test out the idea. How would we go about adding the forbidden one? I'm guessing a new one would have to be designed to fit in with our icon aesthetic? |
|
Oh interesting. I found it in the WordPress design library Figma. It's called "no-swatch". I did increase the stroke width slightly. 
|
Oooh I see, it's not an actual icon but a styled span: |
|
Ok updated to use a span styled like a white, slightly thicker version of the no-swatch design! |
|
Testing really well for me. Massive improvement over what's in trunk. From a design perspective I think we can button-drag.mp4 |
There was a problem hiding this comment.
Looks great, much better and clearer now!
Yes, visually this is looking terrific! No issues or conflicts with the list view anymore, either 👍
I'm a bit hesitant about the performance side of things as the crossfade issue sounds a bit like there could be more event firing happening than is intended. Given that there's been quite a few performance improvements in the editor landing of late, it might be worth double-checking with @ellatrix and @youknowriad to see if the dragover event listener sounds okay to go with for now. Since there can be multiple BlockDraggable components rendered on the page at a time, I'm wondering if it's possible that we're seeing competing callbacks firing, even though each individual callback is throttled?
Other than that performance issue, this is looking great to me! 🎉
Could you share a visual of this? I'm finding it really hard to reproduce even with CPU throttling. Is it just about the CSS transition or the general movement of the chip? |
There was a problem hiding this comment.
Not sure if this is the same as Andy was seeing, but it is a little stuttery for me. A bit hard to show in a screengrab, but here's one I captured with CPU throttling (site editor with the list view opened):
2023-12-19.16.26.30.mp4
When hovering over the paragraph block, the crossed out icon appears to flicker on for a moment before going away.
In terms of the movement of the chip, anecdotally, after re-testing trunk it seems that the drag chip performance when dragging over the editor canvas feels much the same to me between this PR and trunk with CPU throttling enabled.
|
Ooooh ok, that stuttering isn't a performance issue, it's because any block that's not a container currently counts as an area that's not droppable in. I previously tried making sure the droppable areas were containers as a workaround for this, but that didn't work for root level areas, because drop indicators at root level seem to be attached to their sibling block client ids, as opposed to their containers. I couldn't find a reliable workaround for this. Open to ideas! |
|
Nice improvements! |
Not the movement of the chip, it's the crossfade between the two states within the chip: stutter.mov |
What a tricky bit of detail! I haven't had much luck digging further into either of these so far, but something I observed with the stuttering is that it looks like the classnames flicker on and off a couple of times when hovering over a block. So that might be one part of the stuttering? 2023-12-20.15.55.11.mp4 |
|
Update: I have a hunch that I think might fix the stuttering, but in the process further exposes the issue of the disabled status being flagged when dragging over individual blocks. Here's what I could get running locally: 2023-12-20.16.29.45.mp4As far as I can tell, it seems that the use of the Then, further down in the component we'd call these in the In order to get Here's a diff of where I got up to: Diff of hacking around removing the `useEffect`diff --git a/packages/block-editor/src/components/block-draggable/index.js b/packages/block-editor/src/components/block-draggable/index.js
index e5945841d1e..d509337d439 100644
--- a/packages/block-editor/src/components/block-draggable/index.js
+++ b/packages/block-editor/src/components/block-draggable/index.js
@@ -31,8 +31,9 @@ const BlockDraggable = ( {
visibleInserter,
getBlockType,
getAllowedBlocks,
- draggedBlockNames,
+ // draggedBlockNames,
getBlockNamesByClientId,
+ getDraggedBlockClientIds,
} = useSelect(
( select ) => {
const {
@@ -41,7 +42,7 @@ const BlockDraggable = ( {
getBlockName,
getBlockAttributes,
isBlockInsertionPointVisible,
- getDraggedBlockClientIds,
+ getDraggedBlockClientIds: _getDraggedBlockClientIds,
getBlockNamesByClientId: _getBlockNamesByClientId,
getAllowedBlocks: _getAllowedBlocks,
} = select( blockEditorStore );
@@ -62,9 +63,10 @@ const BlockDraggable = ( {
getBlockType: _getBlockType,
getAllowedBlocks: _getAllowedBlocks,
draggedBlockNames: _getBlockNamesByClientId(
- getDraggedBlockClientIds()
+ _getDraggedBlockClientIds()
),
getBlockNamesByClientId: _getBlockNamesByClientId,
+ getDraggedBlockClientIds: _getDraggedBlockClientIds,
};
},
[ clientIds ]
@@ -90,63 +92,53 @@ const BlockDraggable = ( {
const blockRef = useBlockRef( clientIds[ 0 ] );
const editorRoot = blockRef.current?.closest( 'body' );
- // Add a dragover event listener to the editor root to track the blocks being dragged over.
- // The listener has to be inside the editor iframe otherwise the target isn't accessible.
- // Check if the dragged blocks are allowed inside the target. If not, grey out the draggable.
- useEffect( () => {
- if ( ! editorRoot || ! fadeWhenDisabled ) {
+ const onDragOver = ( event ) => {
+ if ( ! event.target.closest( '[data-block]' ) ) {
return;
}
- const onDragOver = ( event ) => {
- if ( ! event.target.closest( '[data-block]' ) ) {
- return;
- }
-
- const targetClientId = event.target
- .closest( '[data-block]' )
- .getAttribute( 'data-block' );
-
- const allowedBlocks = getAllowedBlocks( targetClientId );
- const targetBlockName = getBlockNamesByClientId( [
- targetClientId,
- ] )[ 0 ];
- const dropTargetValid = isDropTargetValid(
- getBlockType,
- allowedBlocks,
- draggedBlockNames,
- targetBlockName
+ const targetClientId = event.target
+ .closest( '[data-block]' )
+ .getAttribute( 'data-block' );
+
+ const draggedBlockNames = getBlockNamesByClientId(
+ getDraggedBlockClientIds()
+ );
+
+ const allowedBlocks = getAllowedBlocks( targetClientId );
+ const targetBlockName = getBlockNamesByClientId( [
+ targetClientId,
+ ] )[ 0 ];
+ const dropTargetValid = isDropTargetValid(
+ getBlockType,
+ allowedBlocks,
+ draggedBlockNames,
+ targetBlockName
+ );
+
+ // Update the body class to reflect if drop target is valid.
+ // This has to be done on the document body because the draggable
+ // chip is rendered outside of the editor iframe.
+ if ( dropTargetValid && ! visibleInserter ) {
+ window?.document?.body?.classList?.add(
+ 'block-draggable-invalid-drag-token'
);
+ } else {
+ window?.document?.body?.classList?.remove(
+ 'block-draggable-invalid-drag-token'
+ );
+ }
+ };
- // Update the body class to reflect if drop target is valid.
- // This has to be done on the document body because the draggable
- // chip is rendered outside of the editor iframe.
- if ( ! dropTargetValid && ! visibleInserter ) {
- window?.document?.body?.classList?.add(
- 'block-draggable-invalid-drag-token'
- );
- } else {
- window?.document?.body?.classList?.remove(
- 'block-draggable-invalid-drag-token'
- );
- }
- };
-
- const throttledOnDragOver = throttle( onDragOver, 200 );
+ const throttledOnDragOver = throttle( onDragOver, 200 );
- editorRoot.addEventListener( 'dragover', throttledOnDragOver );
+ const startDragOverBehavior = () => {
+ editorRoot?.addEventListener( 'dragover', throttledOnDragOver );
+ };
- return () => {
- editorRoot.removeEventListener( 'dragover', throttledOnDragOver );
- };
- }, [
- draggedBlockNames,
- editorRoot,
- getAllowedBlocks,
- getBlockNamesByClientId,
- getBlockType,
- visibleInserter,
- ] );
+ const endDropOverBehavior = () => {
+ editorRoot?.removeEventListener( 'dragover', throttledOnDragOver );
+ };
if ( ! isDraggable ) {
return children( { draggable: false } );
@@ -176,6 +168,7 @@ const BlockDraggable = ( {
onDragStart();
}
} );
+ startDragOverBehavior();
} }
onDragOver={ scrollOnDragOver }
onDragEnd={ () => {
@@ -187,6 +180,7 @@ const BlockDraggable = ( {
if ( onDragEnd ) {
onDragEnd();
}
+ endDropOverBehavior();
} }
__experimentalDragComponent={
<BlockDraggableChipIt's pretty messy and makes things a little more broken in the process, but just thought I'd share it in case it helps the investigating! Happy to keep looking tomorrow, too. |
|
Thanks for all the testing and screengrabs folks! And thanks @andrewserong for diving in and looking at options ❤️ I think I've fixed it now. The problem wasn't as much in the effect itself - because it can run repeatedly without causing the body classname to get added or removed - but in the conditions we were checking. Because non-container blocks were flagged as non-droppable, dragging over them (particularly the Paragraphs for some reason) was causing the classname to get added and removed more than it should. I changed the check to look at the parent block when the current block isn't a container, and that seems to have done the trick! I can't reproduce the crossfade issue @apeatling noticed anymore, so I think it might have been related? But let me know if you still see it! |
There was a problem hiding this comment.
Nicely done, that's resolved the issue for me!
The problem wasn't as much in the effect itself - because it can run repeatedly without causing the body classname to get added or removed - but in the conditions we were checking
That seems consistent with what I'm seeing in testing now. The effect is still running more frequently than it needs to (i.e. because of the useEffect resulting in multiple handlers being called, it runs at greater than once every 200ms), however every time it runs, the output is consistent, which means there's no longer a flicker of the classname being added and removed 👍
To check how frequently it's being called, I added a let numTimesCalled = 0; definition at the root of the file and called console.log( ++numTimesCalled ); from within onDragOver. It's more obvious while dragging, but it gets called a fair bit:
2023-12-21.16.30.27.mp4
That doesn't seem like a blocker to me, as it seems to already be being called less frequently than once per frame, and removing the useEffect could be a good candidate for a code quality follow-up, rather than something that needs to block the feature getting in.
LGTM, great work with all the follow-ups here and figuring out all this nuanced logic! ✨
|
The only way I can think of to remove the effect is if we can decouple the callback from |
|
Yes, that does sound like a bit of a rabbithole (and a good argument for leaving it for now!). I'm happy to have a hack around at it in the new year if you'd like, but no need to delay this PR any longer for now IMO 🙂 |
|
I just merged trunk into this branch as I think some checks were failing because of the node update. |
What?
Fixes #24174.
If a block isn't allowed to drop in a certain area, the drop indicator shouldn't show.
In addition, the draggable chip changes color and icon when hovering over an area where there are no valid drop targets.
Testing Instructions
Note the transparency of the draggable chip isn't yet quite in sync with the display of valid drop targets. This can be fine-tuned but it would be great to get some initial feedback on the general approach first!Update: changes to draggable chip should now be in sync with showing the drop indicator.
Screenshots or screencast
droppingdisabled.mov
dropping-buttons.mov