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

Always show drag handle for nested blocks, even if single; fixes issue #12831 #15025

Merged
merged 1 commit into from
Apr 24, 2019

Conversation

tmdesigned
Copy link
Contributor

Description

Updates block-mover component to always show the drag handle for blocks that are not top-level (i.e. for blocks that are children of a columns block, a group block, etc.). This allows them to be dragged outside of their parent.

Previously, no drag handle was shown if the block was the only child of its parent.

How has this been tested?

I have only tested this on my local environment (the docker setup that is bundled with the gutenberg repo).

@youknowriad youknowriad added Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Bug An existing feature does not function as intended [Block] Columns Affects the Columns Block [Feature] Drag and Drop Drag and drop functionality when working with blocks and removed [Block] Columns Affects the Columns Block labels Apr 18, 2019
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense and works well. 👍 Thanks for the fix.

It does become a bit tricky to manage using the movers between the nested context and its parent, but I don't think it's specifically related to what's being addressed here.

@@ -44,10 +44,10 @@ export class BlockMover extends Component {
}

render() {
const { onMoveUp, onMoveDown, isFirst, isLast, isDraggable, onDragStart, onDragEnd, clientIds, blockElementId, blockType, firstIndex, isLocked, instanceId, isHidden } = this.props;
const { onMoveUp, onMoveDown, isFirst, isLast, isDraggable, onDragStart, onDragEnd, clientIds, blockElementId, blockType, firstIndex, isLocked, instanceId, isHidden, rootClientId } = this.props;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is excessively long and could do to be split across multiple lines. I'd not consider it a blocker, but a good idea for a future refactoring.

const { isFocused } = this.state;
const blocksCount = castArray( clientIds ).length;
if ( isLocked || ( isFirst && isLast ) ) {
if ( isLocked || ( isFirst && isLast && ! rootClientId ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic isn't entirely clear for what it is we're doing here with ( isFirst && isLast && ! rootClientId ), though the same could have been said prior to your changes. I could see this being improved with either a code comment which explains the purpose, or by separating it out as a separate named function which clearly describes its intent.

Not a blocker.

@aduth aduth merged commit 7c18112 into WordPress:master Apr 24, 2019
@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Drag and Drop Drag and drop functionality when working with blocks Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants