Skip to content
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

Refactor the draggable component to avoid passing around unnecessary props #18756

Merged
merged 3 commits into from Dec 5, 2019

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Nov 26, 2019

This PR is extracted from #18685

It does a few things:

  • Remove unnecessary indirection: The DragIcon component.
  • Move all Dragging related checks to the BlockDraggable component.
  • Avoid passing Dragging related component from the Block component down to its actual usage.

I also think this PR makes it easier to support multiple blocks dragging in the future if needed.

Testing instructions

  • Is the refactor worth it?
  • Is dragging blocks still working as intended.
@youknowriad youknowriad self-assigned this Nov 26, 2019
@youknowriad youknowriad requested a review from nosolosw Nov 26, 2019
@youknowriad youknowriad force-pushed the refactor/draggable-component branch from 7cce027 to 760a925 Nov 26, 2019
@@ -196,7 +196,6 @@ class BlockList extends Component {
blockClientIds,
rootClientId,
__experimentalMoverDirection: moverDirection = 'vertical',
isDraggable,

This comment has been minimized.

Copy link
@nosolosw

nosolosw Nov 28, 2019

Member

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.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 28, 2019

Author Contributor

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';

This comment has been minimized.

Copy link
@nosolosw

nosolosw Nov 28, 2019

Member

It's on! :trollface:

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.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 28, 2019

Author Contributor

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>)

This comment has been minimized.

Copy link
@nosolosw

nosolosw Nov 28, 2019

Member

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.

Copy link
Member

nosolosw left a 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

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 4, 2019

Added a potential dev note about the removal of isDraggable.

@youknowriad youknowriad force-pushed the refactor/draggable-component branch from 9f8d6ab to 798a830 Dec 4, 2019
@youknowriad youknowriad force-pushed the refactor/draggable-component branch from c26425d to 2fc44aa Dec 5, 2019
@youknowriad youknowriad requested review from nerrad and ntwb as code owners Dec 5, 2019
@youknowriad youknowriad merged commit 9fa7ee3 into master Dec 5, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the refactor/draggable-component branch Dec 5, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
scruffian added a commit to scruffian/gutenberg that referenced this pull request Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.