Fix navigation link block dragging error #30219
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Regression
|
Size Change: -1.64 kB (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
ellatrix
reviewed
Mar 25, 2021
| // Don't select the element while dragging. | ||
| 'is-selected': isSelected && ! isDraggingBlocks, | ||
| 'is-editing': isSelected || isParentOfSelectedBlock, | ||
| 'is-selected': isSelected, |
There was a problem hiding this comment.
Why do we assign the is-selected class when useBlockProps is responsible for this?
And could we also revert the change here: 748c632#diff-4501da117949f251043d70ffccf02c54af2a99fb80ccd33eb2d40175acdfc833L153?
There was a problem hiding this comment.
I don't know to be honest. Might just have been a visual thing.
It was originally added in this PR - #17832 - which has a lot of comments, it might have been discussed in there at some point.
I'll remove that line.
There was a problem hiding this comment.
Ah, sorry, should have linked this one too: 748c632#diff-4501da117949f251043d70ffccf02c54af2a99fb80ccd33eb2d40175acdfc833R165
There was a problem hiding this comment.
Have removed both of the modified lines, and it still seems to work, so hopefully there's no drama from that change (cd3aa0c).
There was a problem hiding this comment.
If there's a problem we look for a pure CSS solution. :)
ellatrix
approved these changes
Mar 25, 2021
|
Would be good to also remove this change 748c632#diff-4501da117949f251043d70ffccf02c54af2a99fb80ccd33eb2d40175acdfc833R165 |
|
If we do want to make style changes based on a block being dragged, we should use a dragging class in combination with e.g. |
7 tasks
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #30177.
The thing that triggers the error is the call to the
isDraggingBlocksselector. I'm not sure why yet. I'll do some more digging to try to fix the root cause.But this can serve as an intermediate fix.
To resolve the issue I removed a bunch of code related to that selector, and I don't really know what some of it does. It looks like one use of
isDraggingBlockswas to hide the appenders in the nav block when dragging. I don't think it's a big issue if these are visible again.The other cases modify the
is-selectedandis-editingclassnames. It's unclear to me what that might do, but it might be that there was a need for this originally, but now there isn't (the feature still seems to work ok without this).How has this been tested?
Try dragging a navigation link (including into a nested submenu)
Expected result: The block should be drag and droppable.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: