-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Site Editor List View: Fix the width of the list view to 350px #49508
Conversation
Fast work, very cool! Took it for a spin: I'm not seeing the on-hover scrollbars. The mixin is a bit fiddly to get to work, it needs to be applied to the right element for it to work. It should be working in the site editor when you have a lot of templates: One note, we probably want to add a responsive auto width to work on tablet and mobile breakpoints: |
Size Change: +93 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
…rollbar hover work
@jasmussen thanks for the super fast review. I have moved things around a bit. Try now! |
I'm not sure what's wrong with my env, I can see the diff looking right, but pulling or checking out the branch fresh I'm not seeing any difference from before. Curious 🤔 |
This is caused by this line of CSS which is creating space for the scrollbar: |
This should only reserve space when scrollbars are set to be permanently visible, and in that situation I think it's fine to have that extra space, it usually avoids layout shifts. |
The problem is that line is added by the mixin, so maybe we should remove it from there? |
No I think it's both appropriate and stable, it's needed for the firefox and standards based scrollbar style. |
I added the colors to the mixin so I think this should be good to go if anyone else agrees :) |
Trunk doesn't use the mixin in this panel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of preventing the list view width from growing (and defaulting to horizontal scrolling), but I'm also wondering if we can do more to prevent users from encountering that problem. In the site editor, it's very common to go to deeper levels of nesting, so would it be worth revisiting the amount of left margin we're adding for each indentation level? At the moment, it appears to be using the icon size to determine the margin for the next nesting level.
I thought I'd hack around a bit, and here's how it could potentially look if we reduced it to half of the icon size:
On the left: trunk
, on the right: margin-left
uses half of the icon size for its calculation:
To play around with it, in this block of code, I swapped $icon-size
for math.div($icon-size, 2)
:
gutenberg/packages/block-editor/src/components/list-view/style.scss
Lines 367 to 376 in 46969c9
@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); | |
} | |
@else { | |
margin-left: ( $icon-size * $i ); | |
} | |
} | |
} |
I wonder how easy it would be to do this dynamically, so the indentation is only reduced when needed. |
@andrewserong I'd say that's worth exploring separate to this PR, but I have been wondering the same thing :) We can save a lot of room by reducing the indentation like you suggested, and also the gaps between icons/label: The loss of alignment is a shame, but it doesn't bother me as much as the horizontal scrolling. Especially as the alignment is inherently wonky due to the inconsistent icon sizing (most are 16px wide, but some are larger). |
Separate PR sounds good to me. I'm happy to have a go at it if no one beats me to it! |
I still can't get this branch to compile and show the actual custom scrollbar: For reference, the mixin is based on this codepen, which uses a standards based custom scrollbars for Chrome and Firefox, and shows the scrollbar always but only on hover, like so: That's a dark mode version of course, and it needs to be light mode compatible for the list view. But I still think it's a decent solution to the ever scrolling nesting hierarchy. Reduced indentation can work, but it affects readability (icons don't line up with text) and optimizes for super deep nesting, which also may not be ideal. See also in the top GIF how we're already collapsing the nesting margins once we reach a certain depth. I think scrollbars are sort of inevitable. Resizing was mentioned somewhere, that's a cool idea too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After giving this a re-test, I think it'd be worth trying but without custom-scrollbars-on-hover
. Unfortunately the hover behaviour for revealing scrollbars doesn't play very nicely with horizontal scrollbars for users who need to click on the scrollbar itself, as the scrollbar can appear well below the current area of the list. With this rule, the user has to mouse over the content area before then moving the mouse down to the scrollbar, which can be awkward depending on the size of the list.
Here's where folks can get stuck:
2023-04-13.10.29.51.mp4
In the above video:
- In a shorter-height list, if you mouse over the lower area of the list, the scrollbars are not revealed
- If you mouse over the area of the list, then the scrollbar is revealed, but you then have to mouse directly down to the scrollbar, or it disappears
For the list view, would it be better to leave it as overflow: auto
but without custom-scrollbars-on-hover
, so that it's easier to find the scrollbar?
@jasmussen in the screengrab from your testing, I think you might have been testing in the post editor? This PR only affects the site editor. The post editor already has a horizontal scrollbar as of #49611 (the post editor was made to be fixed width in 6.1 I think — when the document outline was moved to the sidebar, if I remember correctly it was this PR that made it fixed width in the post editor: #44788). |
I'll see if I can get a moment to try out the custom scrollbars in a branch, I feel like the current approaches don't implement it the way I pictured it, where indeed hovering the full list view should reveal them at all times, when they are necessary. The instinct for using those custom scrollbars is that we've done entirely without scrollbars so far, and should indeed optimize for scrollbars showing up being the edgecase. The more light weight appearance would work well in that case, just as they do in the site view of the site editor when template lists grow long. But of course the devil is in the details, it needs to feel just right. The custom scrollbars would also avoid a layout shift with the scrollbar appearing and disappearing. |
Closing in favour of #49793 |
What?
Currently the List View in the site editor will keep expanding the sidebar when opening nested block. This restricts the width of the List View to 350px.
Why?
The sidebar should be limited to 350px so that the rest of the page is still visible. This is the same width as the block inserter.
How?
min-width
towidth
Testing Instructions
Testing Instructions for Keyboard
This is only a visual change.
Screenshots or screencast
Screen.Recording.2023-03-31.at.12.11.07.mov
Note
The
custom-scrollbars-on-hover
doesn't look great but I don't think we should handle that in this PR.