Improve the navigation block setup state / placeholder. #31371
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
jasmussen
commented
Apr 30, 2021
| <span className="wp-block-navigation-link"></span> | ||
| <span className="wp-block-navigation-link"></span> | ||
| <ul className="wp-block-navigation-placeholder__preview wp-block-navigation__container"> | ||
| <li className="wp-block-navigation-link">​</li> |
There was a problem hiding this comment.
The markup and class changes here allow inheritance, and the ​ character is a zero width space which ensures the menu item has the correct height: to inherit line height the element has to have content inside.
jasmussen
commented
Apr 30, 2021
| @@ -46,8 +46,8 @@ function Placeholder( { | |||
| let modifierClassNames; | |||
| if ( typeof width === 'number' ) { | |||
| modifierClassNames = { | |||
| 'is-large': width >= 320, | |||
| 'is-medium': width >= 160 && width < 320, | |||
| 'is-large': width >= 480, | |||
There was a problem hiding this comment.
320 is not "large", and the container query system is less useful as a result.
This is nevertheless a change that makes it worth testing other containers that might be targetting the is-large class.
This was referenced Apr 30, 2021
|
Size Change: +54 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
I'm testing inside the columns and the focus behavior feels a little weird. It felt weird at first that it shows the placeholders when it isn't focused. I think the reasoning behind it is that we want it to be visible when someone is working on other blocks, but mutable when they are working on the nav. What was weird to me was that when the column is focused, the placeholders go away. Here's a video where I first click on the nav to make the placeholders disappear, then select the column. I would expect the placeholders to appear when the column is selected, since I would be adding blocks adjacent to the nav instead of inside the nav. Screen.Recording.2021-04-30.at.10.24.36.AM.movIs that the intended behavior? |
|
Yeah that's a good thought. When I built this, it was intended, but the shift as you demonstrate, isn't that useful after all. Let me take another look, and thank you for the review 👌 |
jasmussen
force-pushed
the
try/fix-nav-placeholder
branch
from
d120885 to
c0fbdb4
Compare
|
Rebased, and pushed a fix so that the setup state doesn't wrap. Before:
Note the search icon wrapping After:
|
|
I think this one is in a good place, and is likely going to be close to the final expression of the "skeleton indicators" idea for setup states. If this one works, such skeleton indicators can be used in other places. If it doesn't, I think we need to rethink and use real content. Perhaps drop the setup state entirely and default to inserting the page list block. This would have different consequences, and would require some thought and time in the oven. But let's try this first. |
- Bring back Placeholder component for responsiveness. - Unstyle some things. - Tweak placeholder skeleton to inherit styles and match height of actual menu items.
jasmussen
force-pushed
the
try/fix-nav-placeholder
branch
from
c0fbdb4 to
e68965d
Compare
|
Changes look good. I still see the issue that @georgeh mentioned. If I set "Start Empty" in one column, when that column is focused placeholder goes away. |
|
I'm only seeing "intentional" behavior:
Which is that the gray blobs disappear when you select the empty navigation block. I see that inside and outside of columns. I say intentional in quotes because the gray blobs haven't been quite as succesful as I had hoped, and I'm pondering a separate followup to rethink them yet again. |
|
I think it looks okay when only the "Add navigation button" is visible in empty navigation. Part I was referring to is that the placeholder state completely disappears when a column is selected. CleanShot.2021-05-05.at.14.19.42.mp4 |
|
Oh wow, yes, that was a gnarly bit of logic, excellent catch and thanks for the videos plus patience. Fixing now. |
|
Okay, I think I got it all:
|
|
I'd love a ✅ here ✨ |
Mamaduka
approved these changes
May 6, 2021
|
Thanks. This is an improvement and I'll land it and address any followups. |
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 #31352.
This PR does a number of things:
Placeholdercomponent so we can style according to container widths1 was the key bit missing. The
Placeholdercomponent comes with a "resizeobserver" and assignsis-small,is-mediumandis-largeclasses based on the width of the block. This allows us to stack the setup state in very small contexts.2 and 3 are side-effects of general refactor efforts to remove an "on-select jump", which is better described in #31198 which this PR also replaces. While this onselect thing didn't need to be fixed in this PR, it touched the same code anyway, and made sense to me to include since I was refactoring for various container widths.
4 was the key one. The setup state could be very broken in constrained contexts, for example inside a columns block. This PR fixes that:
5 rephases the "Existing menu" button to instead say "Add existing menu":
Existing menus are created in the separate Navigation screen (both the legacy and the new block-based one). The clarification can hopefully help suggest that the existing menu is meant as a starting point, because there isn't actually a two-way connection between the two. In other words, if you add an existing menu to your navigation block, and later add new menu items on the menu screen, those will not show up in the Navigation block.
How has this been tested?
Please test the navigation block in its setup state. Both the horizontal and vertical versions:
I'm still thinking about whether the skeleton indicators are a good idea at all, and that's something to ponder. But for now they work reasonably well, and most of all they share metrics of the theme now.
Checklist:
*.native.jsfiles for terms that need renaming or removal).