List View: Update drop indicator width to be aware of scroll containers #49786
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
andrewserong
added
[Type] Bug
andrewserong
requested review from
scruffian,
jameskoster,
jasmussen,
apeatling,
ramonjd and
tellthemachines
|
Size Change: +49 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e3d5014. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4685285531
|
Also that it's rendered in a slot outside the scroll container, so it doesn't respect CSS overflow rules. I think this is probably good as a temporary fix, but it does seem like it'll add some extra computational expense. There might be some other alternatives:
|
Oh, cool ideas! Yes, I was mostly imagining this fix as a shorter term solution since it's a bit of an unfortunate visual issue at the moment. If the approach in this PR isn't a blocker, I'd be happy to dig into some of these other alternatives once we're a little further along with some of the other list view improvements. At least on the surface, it looked like most of the ideas I had wound up being some variant of needing to grab the container via |
|
@andrewserong I am still seeing the line extend past in both Chrome and Safari. I'm pretty positive I've followed the instructions exactly and have the changes built correctly. 🤔 |
|
@apeatling thanks for taking this for a spin! It currently only works once the list view extends beyond the vertical edge of the screen. Before this PR can land, I think we'll need to land the fix in #49787 first. Once that's in, I'll give this a rebase and update and then ping for another round of reviews 🙂 |
andrewserong
force-pushed
the
fix/list-view-drop-indicator-width-in-deeply-nested-lists
branch
from
ac0fcb2 to
81f8ade
Compare
|
Update: #49787 has been merged, so I've given this a rebase, and it should be ready for a final review now. |
apeatling
approved these changes
Apr 17, 2023
|
Thanks for re-reviewing! I'll merge this in now. |
andrewserong
deleted the
fix/list-view-drop-indicator-width-in-deeply-nested-lists
branch
25 tasks
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.
What?
Fix an issue where the list view drop indicator's width extends beyond the edge of the list view when the list is deeply nested.
Why?
The cause is two things:
350pxsidebar.So, to support the drop indicator in deeply nested list views, let's reduce the width of the drop indicator so that it doesn't extend beyond the scrollable area.
How?
In the list view drop indicator code, attempt to grab the scroll container, and if the scroll container's width (
scrollContainer.clientWidth) is narrower than the width of the target element, then use that width to calculate the final width of the drop indicator line (factoring in the spacing between the left edge of the container and the left edge of the target element).Known issue:
getScrollContainercurrently only looks at the vertical axis for determining whether a container is present. So for this PR to work, you need a list that extends beyond the height of the screen vertically. Separately to this PR, I'll put up a fix forgetScrollContainer.Testing Instructions
Some heavily nested block markup for testing
Screenshots or screencast