Refactor the draggable component to avoid passing around unnecessary props #18756
Conversation
youknowriad
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
mkaz and
talldan
as code owners
youknowriad
added
the
[Feature] Drag and Drop
youknowriad
force-pushed
the
refactor/draggable-component
branch
from
7cce027
to
760a925
Compare
| @@ -196,7 +196,6 @@ class BlockList extends Component { | |||
| blockClientIds, | |||
| rootClientId, | |||
| __experimentalMoverDirection: moverDirection = 'vertical', | |||
| isDraggable, | |||
There was a problem hiding this comment.
This prop was available so alternative editor implementations could hide the drag icon if they needed (mobile?). Perhaps it's fine to remove, but wanted to flag in case it's not.
There was a problem hiding this comment.
How successful was that? Any editor you know ever used it?
| @@ -19,7 +19,7 @@ import { withInstanceId, compose } from '@wordpress/compose'; | |||
| */ | |||
| import { getBlockMoverDescription } from './mover-description'; | |||
| import { leftArrow, rightArrow, upArrow, downArrow, dragHandle } from './icons'; | |||
| import { IconDragHandle } from './drag-handle'; | |||
There was a problem hiding this comment.
It's on! 
Not a blocker and would definitely go with what you think is better given that I'm not very active lately. However, I still think IconDragHandle has a place here to hide how the DnD logic works and to maintain the same level of abstraction than the other icons. I find it more useful and fitting now that we've removed the onDragStart / onDragEnd boilerplate.
There was a problem hiding this comment.
I agree that it's not all good or bad no matter what we do here.
Personally, while working on it, I find the indirection unnecessary as I was forced to jump between three components (DragHandle, BlockDraggable and the Mover).
maintain the same level of abstraction than the other icons
What other icons you're talking about, I personally find it a bit more consistent this way as the Mover is rendering all its buttons in the same way (directly usiung <IconButton>)
There was a problem hiding this comment.
What other icons you're talking about, I personally find it a bit more consistent this way as the Mover is rendering all its buttons in the same way (directly usiung )
I wish that was the case! But I can't help but feeling that the "children as function" pattern is still odd and IconDragHandle is a nice way to hide it from readers (until they are ready or need to dig in).
However, my concerns are lessened by the fact that, whatever drag component we use, it is not meant to be widely reusable, so I'm ok with this approach.
There was a problem hiding this comment.
Tested this for top-level and children blocks, with fixed toolbar and spotlight mode, with admin and contributors roles. Everything worked as before.
Code-wise, this refactor improves the overall readability: good call on moving the isDragging logic to the Redux state!
I believe the removal of the isDragging prop merits a Breaking change notice in the CHANGELOG, though.
youknowriad
added
the
Needs Dev Note
|
Added a potential dev note about the removal of isDraggable. |
youknowriad
force-pushed
the
refactor/draggable-component
branch
from
9f8d6ab
to
798a830
Compare
youknowriad
force-pushed
the
refactor/draggable-component
branch
from
c26425d
to
2fc44aa
Compare
youknowriad
removed
the
Needs Dev Note
This PR is extracted from #18685
It does a few things:
I also think this PR makes it easier to support multiple blocks dragging in the future if needed.
Testing instructions