Size Change: +199 B (0%)

Total Size: 1.15 MB

_{compressed-size-action}

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 鈥�1锟�7 and I'm happy to help here. Seems like the dream scenario could be:

• Don't show the dashed line placeholder at all if a menu is to be loaded. That might mean deferring the existance of that placeholder until after the initial loading, maybe?
• Solo spinner and dashed-line-placeholder both always have the same height as an eventual menu item would have, to avoid layout shifts. I can probably address this one separately with some tricks.

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 馃檱

There's still a black border around the blink and you'll miss it spinner appearing.

I cannot see the black border. Could you try clean npm i and npm run dev?

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!

I believe I have fixed this with latest commit.

Note that the is wrapped in a

which has blockProps spread onto it's props. This ensures that layout is consistent. However I imagine that the Nav has a "selected" state and perhaps that is what we're seeing?

I'm still seeing the black border, which suggests I'm seeing an unstyled Placeholder component with the spinner inside it. I'm also using TT2.

Can you try the use case where you reload a page with a selected menu?

Not sure what I'm doing wrong here 馃

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 鈥�1锟�7 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.

The fix works perfectly on my end.