Navigation: Fix issue where block collapses to zero height when empty. #38439
Conversation
jasmussen
added
[Type] Enhancement
|
Size Change: +85 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
I am going to review this today. Sorry I'm a bit behind this week! |
There was a problem hiding this comment.
Overall this is yet another great UX improvement. Thank you so much for iterating on these set up states - it makes a huge difference.
I followed your steps and everything worked as expected. I left a few notes.
The only thing I'd like to see fixed is the "loading" state of the block when you first insert it. Again we get a collapsed view. Would be better to show what you have elsewhere. To replicate:
- Create a New Post - note it must be new else you'll have the data cached and the loading state won't happen.
- Use Devtools to enable a slow network (see video).
- Add the Nav block.
- See collapsed loading state.
Here's a video showing that:
Screen.Capture.on.2022-02-04.at.06-58-20.mov
This might be outside the scope of this PR because the loading state is due to the block loading stuff like Menu / Pages data to show in the placeholder. I think we can show the Start empty button much sooner and simply reveal the additional buttons once the data arrives. That's the whole point of a dynamic/reactive UI after all 😄
| // In this state, the block is entirely empty and will collapse to zero height unless there's | ||
| // content inside. The below rule lets the placeholder stay in an invisible state to avoid | ||
| // collapsing and shifting the layout. | ||
| // @todo: This fix could be augmented with showing the generic appender when there are no |
There was a problem hiding this comment.
Yes! Because the experince once you click "Start empty" isn't great.
Why? Because before we clicked we had a nice clear indicatino of whether the block boundaries were in the canvas. Afterwards we're left with a blank canvas and a tiny + icon
Screen.Capture.on.2022-02-04.at.06-52-50.mp4
I think this is probably disorientating for users.
There was a problem hiding this comment.
I mentioned this in my comment below, but all this suggests to me we might want to shelve this PR and look at something like #38337 instead.
There was a problem hiding this comment.
I wouldn't say you should shelve the PR. I say it's an improvement. Once merged we should raise a follow up which adds the button inserter into the block when it's empty.
That way when you click Start empty you see a clear next step (i.e. the button appender) rather than a disorientating (visually) empty block.
What do you think?
| // content inside. The below rule lets the placeholder stay in an invisible state to avoid | ||
| // collapsing and shifting the layout. | ||
| // @todo: This fix could be augmented with showing the generic appender when there are no | ||
| // innerblocks, a la how the Row block does it. |
There was a problem hiding this comment.
Nit: personally I like the note about this targeting inner blocks but the reference to Row doesn't help much. Perhaps just explain what the code is doing for us?
There was a problem hiding this comment.
Alright, I rebased and tried to clarify the comment here. Thanks again!
I definitely agree. I understand that there are some inner mechanisms here, but the layout shifts they create are a bit frustrating. The challenge here is that a) it has to load, and b) the final height of the block isn't known until an actual menu item is inserted, because the height is dependant on font, size, lineheight and a bunch of other properties. In some past iterations a somewhat similar issue was present, which I worked around by outputting a hidden dummy menu item in the setup state — same markup and classes as a real menu item, therefore taking up the same height, but zero width and therefore effectively invisible. I think we can likely do something similar here, but I'm not sure how to accomplish this with the Another option is to skip straight to the generic appender solution, which I mentioned in the CSS comments, essentially what #38337 does for Buttons. What do you think? |
Absolutely. Going to see if I can understand what's happening in placeholder loading state (which is what I'm referring to) right now. We may be able to skip a lot of it and render something early.
I don't think we can use that in preference to the placeholder / setup state. We still need those options upfront. But I do think it could be used to make the initial experience after hitting |
👌
Just to clarify, I mean that when the nav block is empty (but saved), we can show the white bordered plus, like the Row block does, that way it won't technically be "empty": |
jasmussen
force-pushed
the
fix/navigation-collapsed-state
branch
from
dece2ce
to
1dac9ad
Compare
|
keeping an eye on this PR since #38337 was mentioned |
Thanks for looking! I saw your PR making the Buttons block behave more like the Row block, having the white/bordered inserter appear in its empty state. That's one of the paths forward I'd like to see for the Navigation block as well, and it could potentially obviate the need for the CSS in this PR. |
|
Is there still any interest in this one? |
|
I'll close this one for now, we can always revisit. |
|
For reference this is what currently happens when you create a completely empty block and it is selected. When the block is not selected and it's empty you see the placeholder.
|
|
Oh nice, looks like the original issue I was seeing has disappeared on its own! All the more reason to not need this. Thanks! |
Description
Followup to #37099. When a navigation block is entirely empty, i.e. you click "Start empty", you can land in this collapsed state:
It's technically accurate in terms of the block literally being empty, but it's not a great user experience. We should look at doing what the Row block is doing, and show the generic in-canvas appender (black plus in a bordered white box) when there are no innerblocks. In the mean time, this PR provides a guardrail so it doesn't collapse or shift between selected and empty state:
Testing Instructions
Checklist:
*.native.jsfiles for terms that need renaming or removal).