Update list view spacing #60713
Update list view spacing #60713
Conversation
jameskoster
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -1.19 kB (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
This looks and feels very nice, @jameskoster! I'll keep using it in development but first impression is a big improvement. |
|
It's perhaps just a bit tight horizontally, between the icons and text. What about bringing that back to 8px?
|
|
A bigger question though: if we do this, should we do it elsewhere with other lists? We're at a point where there's a bit of consistency across most similar components throughout the experience—but this would depart from that. Is that ok/expected? I'm not entirely sure, but I welcome others' thoughts. Off the top of my head, we're using similar metrics on the following lists:
|
There was a problem hiding this comment.
Thanks for exploring this, it's looking pretty good to me so far, and overall I like the idea of the increased information density, it'll really help avoid scrollbars for those more deeply nested lists, and if it work well we might not even need to eventually allow the list view to be resized horizontally (#53096) 👍
It looks like there are a couple of other areas that will also need updating. In addition to the ones I've commented on, there's also:
And in list-view/style.scss, the displacement classes use 36px or -36px for the offsets. I believe these will also need to be updated, as currently there's a vertical jump when dragging up and down, which I think is due to the 4px or so difference now:
2024-04-15.10.29.40.mp4
| @@ -211,7 +211,7 @@ | |||
| display: flex; | |||
| align-items: center; | |||
| width: 100%; | |||
| height: auto; | |||
| height: $button-size-compact; | |||
There was a problem hiding this comment.
Because there are other areas in JS that need to be updated whenever this value is updated, I'm wondering if it'd be better to hard-code this to a specific pixel value? Just thinking that if $button-size-compact were to change at any point, it could be easy to miss that certain JS areas also need updating.
| @@ -475,22 +475,16 @@ $block-navigation-max-indent: 8; | |||
| ($block-navigation-max-indent - 1); | |||
There was a problem hiding this comment.
I think this one will also need updating? Currently the max indent level appears to be off:
There was a problem hiding this comment.
For sure. We might consider increasing $block-navigation-max-indent as a part of this work too.
| // When updating the margin for each indentation level, the corresponding | ||
| // indentation in `use-list-view-drop-zone.js` must be updated as well | ||
| // to ensure the drop zone is aligned with the indentation. | ||
| @for $i from 0 to $block-navigation-max-indent { | ||
| .block-editor-list-view-leaf[aria-level="#{ $i + 1 }"] | ||
| .block-editor-list-view__expander { | ||
| @if $i - 1 >= 0 { | ||
| margin-left: ($icon-size * $i) + 4 * ($i - 1); | ||
| margin-left: ($grid-unit-20 * $i); |
There was a problem hiding this comment.
Regardless of where we end, we should add a CSS comment to explain some of this math so its a bit easier at a glance, such as // 16px * 1.
|
I'm here for tightening this up a bit. But this seems a tad too tight. I'm I'm not wrong in the math, we're going from 28px to 16px, is that right? Can we try 24px? Or 20px? |
|
If we were to be consistent with other lists that would mean increasing items in list view to 40px height, which would be a move in the wrong direction imo. So long as the metrics follow standards (4px baseline, 24/32/40px heights) I think it's okay for there to be differences (and occasional exceptions) if they optimise the UX for the semantic purpose of the element. |
|
32px height seems good to me. I was specifically referring to the indentation from the left, that feels a bit too narrow. |
|
Sorry I was replying to Rich. Your comments hadn't loaded for some reason, probably because I left the tab open over the weekend. Anyway... Current indentation is 24px for the first child. Subsequent indentation is 24 * nesting level + 4. On this branch it's 16px for the first child, and multiples thereof for subsequent children. If we bump that up to 20, and increase the spacing between icon + label as Rich suggested we get this: 
I'll push that to the branch for y'all to try. |
|
Looks a bit jumbled up. The nested indentions should all be aligned under parent icons, like they are in trunk.
|
| @@ -488,9 +480,9 @@ $block-navigation-max-indent: 8; | |||
| .block-editor-list-view-leaf[aria-level="#{ $i + 1 }"] | |||
| .block-editor-list-view__expander { | |||
| @if $i - 1 >= 0 { | |||
| margin-left: ($icon-size * $i) + 4 * ($i - 1); | |||
| margin-left: (20px * $i); | |||
There was a problem hiding this comment.
If we use 24 here, and on line 474—and 6px on line 280, we have bit better vertical alignment
There was a problem hiding this comment.
But I still wonder if it's just a bit too tight vertically, with the icon sizes we currently have. Maybe it's fine?
| Before | After |
|---|---|
 |
 |
There was a problem hiding this comment.
I feel like it's fine. We could add a 4px gap between the 32px items, but I wonder if that would be a worse experience in practice, since you could click between items and not select anything?
There was a problem hiding this comment.
The motivation here is to fit more blocks into the container before scrolling occurs to make navigating complex documents easier, so I don't think we should increase the vertical spacing.
I'm not a fan of using values outside of the 4px baseline unless absolutely necessary, so would prefer to stick with 4 or 8px for the space between icon + label. I still think 4px is fine, personally, given the motivations outlined above. But will leave it at 8px if there's a strong feeling about that.
I like the other suggestions 👍
There was a problem hiding this comment.
with the icon sizes we currently have
Not for this PR, but fwiw I wouldn't be opposed to trying slightly smaller icons here.
There was a problem hiding this comment.
I wouldn't mind that, but I don't think the icons scale sharply.
It's tricky to balance each part with the whole, but I'm feeling this.
|
I made a few tweaks to align the icons, labels, and chevrons more vertically.
|
|
Looks great. |
|
Thanks for pushing those changes. I appreciate the desire to align certain elements, but we're now only saving 8px horizontal space vs trunk which seems a shame to me. For such a practical UI the space seems more valuable than some of these alignment details, if I'm honest. It's not a hill I'll die on though, this would still be an improvement. Two questions though, here: 
Adjusting those two details produces the following, which still respects the alignment (so far as I can see) while also reducing some horizontal space through nested levels, and avoids diverging from the 4px base: 
|
|
That's 8px per item, that compounds a lot, so it could be a good place to start. That said, those two pixels we can throw in for good measure too, to be sure. |
I added it to balance the expander horizontally between the viewport and block icons. It always adds just a bit of space between the chevron and the icon—as only the chevron is triggers the opening and standing a bit more independently makes it clearer to select.
A tiny detail, but something I couldn't not try. :) An alternative (or addition tweak) could be to reduce the padding around the panel, though this would be the only case I'm aware of with a 4px gutter around the items.
|
There was a problem hiding this comment.
This is feeling really close to me, great work everyone!
Just left a comment on very deeply nested items (around 8 levels deep) where the label looks like it can get a bit too truncated due to the removal of the min-width. I hesitate a little bit in flagging this, as I'm such a big fan of how this PR avoids horizontal scrollbars, but I'm wondering if we need to add back in a bit of min-width to ensure enough of the label is always readable?
| @@ -379,8 +378,10 @@ | |||
| } | |||
| } | |||
|
|
|||
| .block-editor-list-view-block-select-button__label-wrapper { | |||
| min-width: 120px; | |||
There was a problem hiding this comment.
I could be wrong, but it looks like the removal of the min-width can result in very truncated / unreadable labels at the deepest nesting level. This is a bit of a contrived example, but in the following, there's a deeply nested Group block with a custom html anchor:
At this nesting level and with the lock icon, the icon winds up overlapping the settings menu button a bit, too:
There was a problem hiding this comment.
I pushed some spacing tweaks to try based on discussion in the issue. Here's what I now see at the deepest nesting level:
I greatly appreciate the lack of horizontal scrolling. The truncation is still a bit aggressive when an anchor is set, but it might be passable?
Not a strong feeling, happy to re-instate the min-width.
There was a problem hiding this comment.
That's feeling much better to me. It's a difficult balance to strike, but for my tastes, the absence of having to scroll horizontally beats out the desire for a large minimum width of the label. I think this would be good to try, and it sounds like it'd be a fairly simple follow-up to re-instate the min-width if folks find it confusing with this level of truncation 👍
This is how it looks for me:
I see it now! Not a big issue but that alignment is kind of lost when you select a block. A 4px gutter could be worth a try... the new dropdown menu components use a 4px gutter. |
There was a problem hiding this comment.
This is testing very nicely for me, and visually looks great IMO. It's very pleasing to see a greater level of nesting supported without having to scroll horizontally, while still having the icons and nesting levels lining up well visually.
✅ Code change looks good
✅ List view displacement while dragging works as on trunk
✅ Dragging up and down the nested hierarchy works as on trunk in both LTR and RTL languages
✅ Windowing logic is working correctly in very long posts
If everyone else is happy with how it's looking, this looks good to land to me! ✨
| @@ -379,8 +378,10 @@ | |||
| } | |||
| } | |||
|
|
|||
| .block-editor-list-view-block-select-button__label-wrapper { | |||
| min-width: 120px; | |||
There was a problem hiding this comment.
That's feeling much better to me. It's a difficult balance to strike, but for my tastes, the absence of having to scroll horizontally beats out the desire for a large minimum width of the label. I think this would be good to try, and it sounds like it'd be a fairly simple follow-up to re-instate the min-width if folks find it confusing with this level of truncation 👍
This is how it looks for me:
|
How are we feeling about this one @WordPress/gutenberg-design ? |
|
I'm good with it. 🚀 |
|
Sweet. I'll merge so we can try it out for a bit. There's plenty of runway to 6.6 so let's tweak if/when the need arises! |
|
Just noting that this PR had a +5% (approximatively) impact on the "open list view" metric. Do we know what is impacting the performance here? |
|
What might cause that? Could it be that more blocks are visible in List View due to the reduced spacing? This PR just changes some pixel values relating to list view item spacing. |
That makes sense I think indeed. It's fine in that case IMO, especially since it's not a critical impact. |
|
Was it intentional this would affect this view as well; the single navigation within the Site Editor? Feels like perhaps it should not have been, as all other list items in the Site Editor are bigger.
|
What?
Why?
With the site editor becoming more established since 6.5, it's not uncommon to find complex block structures when accounting for templates, template parts, and patterns. Given the current metrics, such structures often result in lots of scrolling around in list view, both vertically and horizontally.
This PR seeks to reduce the likelihood of scrolling by reducing whitespace in the layout.
Testing Instructions
Screenshots