Make Spacer block width adjustable and add it to Navigation block #29133
Conversation
tellthemachines
added
[Block] Navigation
tellthemachines
requested review from
talldan,
shaunandrews,
draganescu and
ItsJonQ
|
Size Change: +218 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Taking a look.
Height adjusts the boundary box. Regarding the stand alone Spacer block. Btw Spacer and Separator block are very much beginning to blur the lines. |
|
This is great, thank you for working on it. Here's what I see:
A few quick thoughts. When in the horizontal nav block, you shouldn't be able to vertically resize the spacer, and I imagine vice versa in the vertical navigation block. The spacer now has a horizontal resize handle, and is not full width, when used inside post content: In the site editor, when you hover the spacer, it shows up as gray:
I don't know why that actually doesn't work in the post editor, it should, I will look into that separately. But that will benefit this PR as well. As for the behavior overall, I generally like how the MacOS finder does it:
The flexible space is supposed to work a bit like a flex "auto" margin, but I couldn't quite show how it worked here. I think we'll probably want to skip that one. But the default size of the spacer is really nice — while there isn't any ability to resize the spacer in the finder, the default size feels harmonious. We should choose a default size for the navigation block as well. It's a bit wide right now. How about defaulting to 72px wide? That's a nice round number 😅 (base12 for life) Let me know if I can help! |
|
Thanks for the feedback @jasmussen ! Yes, it makes sense to only be able to resize in the direction the layout follows. I've changed it so that the Spacer block takes parent orientation from context, and behaves accordingly. It there's no context given, it defaults to vertical. I also set default width to 72px, and had to set a fixed height of 32px when orientation is horizontal, otherwise the height just collapsed. It works ok in the Nav block, but let me know if you have other ideas. |
|
This is what I see:
Honestly that feels completely solid to me, for a V1. Really nice. My only thought is that it would be nice to be able to use VW or % units in the sidebar control:
... same as we have a unit selector for the Cover height. Stellar work. |
|
A real world case. First example is a paragraph block. Second example shows adding a horizontal menu. Btw the Paragraph block uses the default width. Based on the above screenshot it looks like the navigation block does not use the same width as the paragraph block. One can also compare it to the below large image. It would be nice for default width of various blocks where it is natural to do so use the same default width, so one can easier align contents of one block with another. |
Yes! That will be added separately; see discussion in #22956. I'm updating the tests and reckon this is ready for a code review. |
There was a problem hiding this comment.
@tellthemachines this is a very good start and it allows us to experiment with layout creation in this complicated block. I had one question about the useEffect that set updateHeight( 0 );.
| { | ||
| 'is-selected': isSelected, | ||
| } | ||
| ) } | ||
| size={ { | ||
| height, | ||
| width, | ||
| height: 32, |
There was a problem hiding this comment.
Awesome PR, this is going to allow so many new designs! 🎉
I'm wondering if there might be cases where 32px is too much for a fixed height. Although adjusting vertically to the parent height is trickier than doing so horizontally, ideally, it would behave like the vertical spacing, where width is omitted. Maybe something maybe worth exploring later, right?
There was a problem hiding this comment.
Solid observation. 24px feels like a plenty tappable size that also goes well with the "minimum" size of the navigation block, as it's the size of the appender.
|
Just gave this a spin, and it works super well. Thank you so much — this is a great enhancement. Only thing that stuck out to me was the extra margin that's applied to the spacer when used in the vertical Navigation block:
This margin seems to appear everywhere the spacer block is placed, but I noticed this PR overrides this extra margin when in the horizontal nav. Perhaps we don't need the margin at all, and should just remove it entirely? Or, maybe we extend the override to apply to both horizontal and vertical nav variants? |
There was a problem hiding this comment.
Great work here @tellthemachines ! This should make it much easier to build navigation menus.
I found a corner case when transforming from a horizontal to vertical variation. The spacer gets a height of 1, which makes it rather difficult to select. This is non-blocking though since I think it's fine to fix up in a follow up PR.
spacer.mp4
| ) } | ||
| { orientation !== 'horizontal' && ( | ||
| <RangeControl | ||
| label={ __( 'Height in pixels' ) } |
There was a problem hiding this comment.
General question from me, when do we normally add inline translator comments?
There was a problem hiding this comment.
Comments are used to add information about the string's placement or meaning in situations where the text content, taken out of its context, might not be clear enough. This is because translators only see a list of strings and don't necessarily know whereabouts in the UI they might live.
There was a problem hiding this comment.
Gotcha, so more of a case by case basis if it's pretty short/generic 👍
| onChange={ updateWidth } | ||
| /> | ||
| ) } | ||
| { orientation !== 'horizontal' && ( |
There was a problem hiding this comment.
Minor: It's a bit more readable if we check for orientation === 'vertical'. Though I understand if we're trying to guard against malformed data.
There was a problem hiding this comment.
Yeah, the problem here is the orientation prop doesn't always exist, and when it doesn't, the default should be vertical. So I thought this was the least messy way of expressing that.
|
Thanks for the reviews everyone! I'm going to go ahead and merge this, and deal with remaining issues (horizontal to vertical height issue, and margin on resize container) separately. |
|
Great improvement! |
|
👋 We recently spotted that the changes of this PR require modifications in the native version because the Spacer block is no longer working, it generates an error just by adding the block to the content. I opened this issue which I'm going to take a look soon so I'd appreciate it if someone could help me in case I have any doubt or if I require to change anything from this PR, thanks! |
The problem is not really caused by the changes from this PR, however they uncovered an issue we have in the native version. Basically the I have a PR with the fix that adds the block context as in the web version 🎊 . |
Description
Fixes #24028.
Adds width adjustment to the Spacer block. Currently only fixed width in
pxis available; #22956 will address making it responsive.Adds the Spacer block as an option to Navigation and Navigation link blocks.
Horizontal Navigation:
Vertical Navigation:
Open questions:
Standalone Spacer block:
How has this been tested?
Screenshots
Types of changes
Checklist: