-
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
Support drag-and-drop for submenus of navigation blocks #24479
Conversation
Size Change: +3.89 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
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.
Only a partial review, I'll try to get to the rest tomorrow 😄
It's not ideal that you have to drag the block movers which are so far away form the menu item. Perhaps we need to re-evaluate our use of Nonetheless, I tested this locally and it looks smooth! Nice job. One thing I noticed though is that this only works in the Navigation block when inserted into a post or page and not in the Navigation screen that's in Gutenberg → Navigation (beta). Could we fix 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.
Looks good, thanks for working on this! Apologies it took a little while to get around to re-reviewing.
Would be great to make a few final changes as per the comments before merging.
@@ -657,6 +666,8 @@ | |||
|
|||
.is-dragging-components-draggable & { | |||
opacity: 0; | |||
// Use a minimal duration to delay hiding the element, see hide-during-dragging animation for more details. |
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.
It might be good to explain in the comment why the toolbar/popover needs to be hidden, mentioning that it obscures drag events if it remains in position overlapping preceding blocks.
@@ -628,6 +628,15 @@ | |||
} | |||
} | |||
|
|||
// Hide the popover block editor list while dragging. | |||
// Using a hacky animation to delay hiding the element so that dragging can still work. |
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.
Maybe mention that if the element is hidden immediately, dragging won't work, and this is why a delay is required.
body.is-dragging-components-draggable &.is-dragging-within { | ||
> .wp-block-navigation__container { | ||
opacity: 1; | ||
visibility: visible; | ||
} | ||
} | ||
} | ||
|
||
body.is-dragging-components-draggable .wp-block-navigation-link > .wp-block-navigation__container { |
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.
Let's drop the body
part of the selector if possible, usually just class names are used.
@@ -159,7 +160,8 @@ function BlockListBlock( { | |||
'is-focused': | |||
isFocusMode && ( isSelected || isAncestorOfSelectedBlock ), | |||
'is-focus-mode': isFocusMode, | |||
'has-child-selected': isAncestorOfSelectedBlock, | |||
// Don't select child while dragging. | |||
'has-child-selected': isAncestorOfSelectedBlock && ! isDragging, |
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.
Not sure the comments here add much, I think we could either remove them or add some more detail on why those classes aren't applied when dragging.
@noisysocks I think this is just removing the one line here: I think that can be done in a separate PR. But even then, the nav page still uses a 'top toolbar' style toolbar, we'd have to drop that as otherwise it looks similar to the |
Oups! Again, I see green I click. So @kevin940726 said he'll update this in a separate PR to address feedback from @noisysocks and @talldan 🚀 |
…)" This reverts commit 748c632.
Description
Close #23874, enable drag-and-drop for submenus in navigation block.
How has this been tested?
Currently only manually testing are available.
Screenshots
Types of changes
New feature
Checklist: