Clear block selection in the navigation editor when clicking editor canvas #28382
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] Enhancement
talldan
requested review from
kevin940726,
noisysocks and
tellthemachines
as code owners
|
Size Change: +179 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
talldan
force-pushed
the
fix/clear-block-selection-on-click-outside
branch
from
b39b99c to
5cb3f18
Compare
There was a problem hiding this comment.
This is working well, apart from the issue with the scrollbar mentioned below 😬
I'm wondering if we couldn't clear the selection when clicking anywhere inside edit-navigation-layout instead of edit-navigation-editor. It's a bit weird that the block stays selected when clicking on "Manage locations" for instance, or on the blank space above the editor.
|
|
||
| // Wp admin top bar becomes less-chunky, so a smaller subtraction is needed. | ||
| @include break-medium() { | ||
| height: calc(100vh - 97px); |
There was a problem hiding this comment.
This line seems to be the cause of some weirdness with the vertical scrollbar flickering in and out of existence when hovering over the list view or opening dropdowns.
There was a problem hiding this comment.
@tellthemachines Thanks for testing, would you be able to share repro steps and maybe screenshots? I didn't spot anything locally apart from some weirdness with the List View when it's big:
There was a problem hiding this comment.
Yup, so it's happening on Chrome/macOS, here's a gif:
It might be related to the horizontal scrollbar, as that's only happening on Chrome too (though that's not specific to your branch).
In testing across environments, I also realised that there can be a notice under the admin bar, and that'll always cause a vertical scrollbar to appear, because the 100vh - x isn't taking it into account:
My suggestion is, instead of using vh, let's leverage the page-specific gutenberg_page_gutenberg-navigation body class and try something like:
.gutenberg_page_gutenberg-navigation {
#wpwrap, #wpcontent, #wpbody, #wpbody-content, .edit-navigation, .components-drop-zone__provider, .edit-
navigation-layout {
display: flex;
flex-direction: column;
flex-grow: 1;
}
}
There was a problem hiding this comment.
That's a lot better, thanks for saving me from CSS hell 😆
This was my first attempt, but the block deselection was still triggering when clicking the toolbar (even toolbar buttons) and other items that shouldn't cause deselection. |
grzim
reviewed
Jan 28, 2021
| display: flex; | ||
| flex-direction: column; | ||
| // Make the navigation layout full-height, accounting for wp-admin top bar and other furniture. | ||
| height: calc(100vh - 146px); |
There was a problem hiding this comment.
do we need to use hardcoded values here?
There was a problem hiding this comment.
I think yes because of the other surrounding stuff from WP Admin which are fixed in size.
There was a problem hiding this comment.
Have removed those after following @tellthemachines' suggestion
|
In my testing (on Edge) a ghost inserter remained: ghost-inserter.mp4This may be out of the scope of this PR? |
| @include break-medium() { | ||
| height: 100%; |
There was a problem hiding this comment.
This isn't working on Safari for some reason:
Using align-self: stretch instead should fix it.
There was a problem hiding this comment.
Nice. Thanks for the suggestion (and finding that issue).
Seems related to #28418. |
talldan
force-pushed
the
fix/clear-block-selection-on-click-outside
branch
from
2eb8d86 to
5217697
Compare
talldan
commented
Jan 29, 2021
Comment on lines
+22
to
+30
| return selectedBlockClientId ? ( | ||
| <Button | ||
| isSecondary | ||
| className="block-editor-skip-to-selected-block" | ||
| onClick={ onClick } | ||
| > | ||
| { __( 'Skip to the selected block' ) } | ||
| </Button> | ||
| ) : null; |
There was a problem hiding this comment.
To explain this change, I spotted an error with the previous code when testing. The component could return undefined which causes React to throw an error. It only seems to be an issue in the navigation screen and noticeable with this PR because the block couldn't be deselected before.
|
I think I've addressed all the changes, hopefully should be good to go! |
draganescu
approved these changes
Jan 29, 2021
tellthemachines
approved these changes
Feb 2, 2021
talldan
force-pushed
the
fix/clear-block-selection-on-click-outside
branch
from
5217697 to
9b5dfc3
Compare
|
Thanks for the reviews, appreciate the help. |
6 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 #22626
Implements the 'block selection clearing' feature in the Navigation Editor.
What is it? In the post and site editor, when clicking in the empty editor canvas blocks are deselected. This feature wasn't present in the Navigation Editor.
Because block selection determines whether a sub-menu is open, it was a bit hard to close sub-menus in the navigation editor, which this solves.
This has required a bit of CSS wrangling as previously the block editor part of the Navigation Editor had a lot of margin around it, and collapsed to the height of its content leaving no empty clickable area.
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: