Improve Nav block loading and placeholder states #38907
Conversation
getdave
added
[Feature] Shortcodes
|
Size Change: +199 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
Thank you for taking a stab at this. Even as is, this is a big improvement. Here are a few gifs. Insert fresh: Nice. 👌 (I hope to look separately at making sure the height of this placeholder state is the same as a menu set in that theme will be, right now it's locked to 48px, which can cause some shifting.) Selecting a menu:
Blink and you'll miss it. It does look like the spinner has a black border around it, that'd be nice to do without. Possible? Reloading (or loading fresh) a page that has a specific navigation already:
This is probably the one that could use the most love. Doesn't all have to be in a single PR, I know how deep the rabbit hole goes as far as the code — and I'm happy to help here. Seems like the dream scenario could be:
|
|
Tons of work in a short time, wild 👌 Took it for another spin. Inserting:
Cool. I wonder if we need to see the spinner there at the end or not, seems like we could do without, but that's not a strong opinion. Selecting a menu from the placeholder:
Nice. There's still a black border around the blink and you'll miss it spinner appearing. As noted, I'd like to try separately to fix the height shifting from placeholder to spinner to menu, so it's one height for all three. Reloading:
Probably still the most difficult flow to get right, and it's looking very close! Basically it looks like there are two spinners now, first an initial basic one, then one inside a bordered box, and then we get the menu. If we could reduce that to just the first spinner, seems like it'd be a shoo-in! |
|
Thanks for super fast reviews 🙇
I cannot see the black border. Could you try clean
I believe I have fixed this with latest commit. |
| @@ -96,7 +96,7 @@ export default function NavigationInnerBlocks( { | |||
| isSelected || | |||
| ( isImmediateParentOfSelectedBlock && ! selectedBlockHasDescendants ); | |||
|
|
|||
| const placeholder = useMemo( () => <PlaceholderPreview />, [] ); | |||
| const placeholder = PlaceholderPreview; | |||
There was a problem hiding this comment.
This appeared to be causing the component to be rendered when it didn't need to be.
I'm not sure this component is rendered often enough to warrant a memo here but I could be wrong.
| @@ -658,29 +693,6 @@ function Navigation( { | |||
| </InspectorControls> | |||
| ) } | |||
| <nav { ...blockProps }> | |||
| { isPlaceholderShown && ( | |||
There was a problem hiding this comment.
These complex loading states have been extracted to separate conditionals each of which returns early with a dedicated render.
| @@ -9,12 +9,11 @@ import classnames from 'classnames'; | |||
| import { Icon, navigation } from '@wordpress/icons'; | |||
| import { __ } from '@wordpress/i18n'; | |||
|
|
|||
| const PlaceholderPreview = ( { isLoading } ) => { | |||
There was a problem hiding this comment.
Loading variant seemed confusing. Not sure why it was necessary.
There was a problem hiding this comment.
PlaceholderPreview previously displayed the gray empty state blobs, and isLoading was the pulse animation variant of that.
Feels like this entire component is redundant now. It does the same thing as the Placeholder, but without any buttons. Maybe it can be deleted, but I haven't checked to see where it's used.
There was a problem hiding this comment.
Not sure why it was necessary.
I should have qualified that statement. I understood why it existed but not it's no longer required due to refactoring.
Maybe it can be deleted, but I haven't checked to see where it's used.
It was also used as the placeholder prop arg for the <InnerBlocks> component which would show when the inner blocks were empty but present and not selected.
I'll check whether it can be ditched. It would certain make things less confusing 😄
There was a problem hiding this comment.
I checked and it's used in the following scenarios:
- The block is unconfigured and not currently selected.
- The block is configured (i.e. has
<InnerBlocks>) but empty and not currently selected.
I think I would prefer to create a follow up whereby we refactor this component away. Trying to do it in this PR is going to create a fair amount of noise which I think will make it even harder to have confidence in the PR.
How do you feel about that? Happy to raise the Issue.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
|
||
| { isResolvingActions && <Spinner /> } | ||
|
|
||
| { showSelectMenus ? ( |
There was a problem hiding this comment.
Now that we've removed the "Add all pages" option, does it even make sense to show the placeholder if there are no existing menus to select? It feels a bit weird to have a placeholder with a single option inside it:
There was a problem hiding this comment.
This is an excellent and pertinent question touching on a larger issue that'd be good to explore. Overall, it would be very nice if we could rid of the placeholder in most cases, such as if you already have a single menu defined, then activate a new theme, just load that first menu automatically, since it's so easy to swap it out from the toolbar anyway.
The other is the behavior of when zero menus are defined. If I understand this correctly, the "Page List" block is currently output automatically on frontends in empty menus, is that right? While useful in principle it does make for a bit of a disconnect between frontend and editor, that could result in confusion: "I'm seeing all my pages here, but not there". In that light, I wonder if we should just preinsert the Page List block in navigation, if no saved menus are created, so the two are in sync? I'm assuming no, based on past discussions on navigation persistence, but I'm suggesting the concept in case it can inspire other ideas. We still don't really have a good way to create patterns for the navigation either.
The new empty state is inspired by Site Logo and Featured Image, and features this dashed line:
Outside of somehow pre-populating an empty block, it seems prudent to at least keep that state around to indicate: there's an empty navigation block here which you can click to configure. If we can have that state, and go directly to the nav inserter on select, in principle that sounds good. But wouldn't that require us to defer the saving to a CPT? Otherwise simply clicking the dashed line nav block would presumable create an empty menu which was then loaded by default? 🤔
I'm still seeing the black border, which suggests I'm seeing an unstyled Can you try the use case where you reload a page with a selected menu? Not sure what I'm doing wrong here 🤔 |
packages/block-library/src/navigation/edit/placeholder/index.js
Outdated
Show resolved
Hide resolved
getdave
added
Needs Technical Feedback
|
Alright, with the help of Dave in a chat debugging session, it became clear that the black border I was seeing was from the Page List block. Which of course I should've probably clarified in my testing steps — my apologies, a beverage is owed by yours truly. All that is to say: I'm now seeing the same: And this is a vast improvement, and not one that should be held up by potential separate Page List block improvements. Thanks Dave, this is good for me. I do still separately want to look at adding a min-height based on menu item text, so we can avoid layout shifts (and find a better alternative to #38439), but that's also separate. |
Addresses - #38907 (comment) - #38907 (comment)
getdave
force-pushed
the
fix/nav-block-loading-states
branch
from
71a3b96 to
2886dac
Compare
|
The fix works perfectly on my end. |
Description
Currently the loading and placeholder states for the Nav block are quite difficult to reason about.
PlacholderPreviewwhich is rendered at both the root block level and within thePlaceholderComponentitself.This PR seeks to solve the above issues by:
setState()s.aria-liveannouncement of block loading progress.🙏 Please note: this PR does not attempt to fix the "freezing" of the UI that occurs when you create a new Menu or select an existing one. That issue is addressed in #38858. The aim of this PR is simply to iterate towards an improved loading state for the block.
Testing Instructions
I recommend testing the scenario on
trunkfirst to get a sense of just how suboptimal the current loading experience is.Now on this PR's branch:
Slow 3Gand add a Nav block.Screenshots
Before
No clear loading state for initial setup or subsequent loading of existing blocks:
Screen.Capture.on.2022-02-18.at.10-42-01.mov
After
Clear loading state for initial setup and subsequent loading of existing blocks:
Screen.Capture.on.2022-02-22.at.11-28-40.mov
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).