Update item height in Navigation menu list views #61141
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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: +126 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 9b26259. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8845892295
|
richtabor
approved these changes
Apr 26, 2024
There was a problem hiding this comment.
Thanks for the ping!
Navigation menus rarely contain as many blocks as documents like pages and templates, so the reduced sizing is unnecessary in this context.
Is this assuming sites with only a few pages? What about larger sites, such as corporate sites, university websites, or others that have deeply nested hierarchies? For these, I think it's still beneficial to go for the higher density. It's quite common to have menus with 20 or 30 total pages (nested under parent-level items) for community sites and groups. The main menu of https://wordpress.org/ is a good example of what I had in mind there.
In my mind, I thought of the list view UI as being different to other items within the site editor or sidebar, so I quite liked the look of the tighter vertical rhythm in these components, too, from #60713.
If we do go with larger spacing, it could be tricky to figure out in a reliable way, as the height is also hard-coded in JS for the displacement here:
which is used to calculate the height of the dragged items here:I suppose one option could be to measure the DOM element in order to set the height for the JS calculation? That could get a bit awkward, though 🤔
Personally, my preference would be to go with the more condensed spacing for the navigation block UIs, both for how it looks, and to reduce the complexity of maintaining two different sets of heights.
|
I think the main issue is the dark sidebar, where the presentation of list view items is very similar to general menu items which makes the subtle difference in height feel a little uncanny / unintentional. Then again, there are other differences besides height:
Not to mention the fact they're totally different elements... You might argue they should have more pronounced visual differences in order to set appropriate user expectations. Keen to gather more feedback on this one. |
I don't think the inconsistency between menus is worth the little gain in visual space we get for the sites that do have lots of menu items. In the standard List View, it's a contained experience, but in the Site Editor navigation you get to list view from a menu. Those variances in menus will make it confusing, regardless of how many menu items you have on a site. Especially as every other list of items in the Site Editor navigation is not small. |
I see your point. To me, they're different interfaces, though, in the sense that the other items in the site editor are for navigating through the menu hierarchy, whereas this is a list of items that folks are rearranging or adding to. I quite like the increased information density (particularly on smaller screens like laptops), but my views on the spacing here are definitely not strongly held! If we'd like to pursue using a different height of list items, and get it working properly with displacement, etc, I think one way we could get the JS side of things working could be to add an Then, for the instances of I think the final thing to do then, to get the height working for displacement, would be to output an additional CSS var in the list view (here) for the single item height, in addition to using the passed in value for the fixed window list here. Then, refactor the CSS rules for the displacement to use that CSS var instead of the hard-coded So, a bit of refactoring would be required, but if we think there's a solid case here for different list views to have different item heights, then I think it's worth incorporating it into the component's API in this way. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#60713 reduced spacing in List View. This also affected Navigation menu list views, both in the block inspector and in the site editor dark sidebar.
Navigation menus rarely contain as many blocks as documents like pages and templates, so the reduced sizing is unnecessary in this context. This PR increases the height of list view items to 40px, which also restores alignment between site editor menu items and list view items.
A trade-off here is that the displacement effect in list view is based on a 32px height, so that doesn't work quite as nicely in this context. @andrewserong is it possible to override that value in this scenario?