Expand block list tree on selection. #35817
Merged
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
|
Size Change: +113 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
try/expand-block-list-tree-on-selection
branch
from
f5ed185
to
9b5e22e
Compare
ramonjd
added
[Status] In Progress
ramonjd
force-pushed
the
try/expand-block-list-tree-on-selection
branch
from
9b5e22e
to
32ab7ae
Compare
|
Opening up for review to see if this is a decent approach. 🙇 |
gwwar
reviewed
Oct 26, 2021
|
Note that If I can land windowing in List View, it will also matter less if the entire component re-renders, so this isn't necessarily a hard stop on this approach. |
gwwar
reviewed
Oct 26, 2021
ramonjd
force-pushed
the
try/expand-block-list-tree-on-selection
branch
from
32ab7ae
to
5cac465
Compare
ramonjd
force-pushed
the
try/expand-block-list-tree-on-selection
branch
2 times, most recently
from
b1c28f7
to
4fb80fe
Compare
ramonjd
commented
Jan 7, 2022
ramonjd
commented
Jan 10, 2022
packages/block-editor/src/components/list-view/use-list-view-open-selected-item.js
Outdated
Show resolved
Hide resolved
ramonjd
force-pushed
the
try/expand-block-list-tree-on-selection
branch
from
6884770
to
bc16cda
Compare
ramonjd
commented
Jan 10, 2022
packages/block-editor/src/components/list-view/use-list-view-open-selected-item.js
Outdated
Show resolved
Hide resolved
ramonjd
force-pushed
the
try/expand-block-list-tree-on-selection
branch
from
bc16cda
to
5869aef
Compare
|
I've abandoned the scrollTo logic in this PR added in 23c762a. I think it's best suited for a further iteration/exploration. There is an issue for it here: #36583 I still believe the changes in this PR represent a UX improvement. At the moment, after selecting a block in the editor, you still have to expand and find the corresponding list item. Before this PR With this PR |
andrewserong
approved these changes
Feb 1, 2022
There was a problem hiding this comment.
This is looking great to me, @ramonjd, nice work figuring it out! I definitely think it's worth getting in, it's a nice little enhancement that makes the list view feel a little more natural.
I tested that it's working well in the post, site, and widget editors (that last one using TwentyTwenty classic theme).
With the limitations that you mention, the only one that I ran into (that I noticed) was toggling the list view open and closed with the same block selected results in the block no longer as showing that it's selected within the list view. Is that what you meant with the first limitation, or is there another one that I missed? Here's a quick screengrab:
Kapture.2022-02-01.at.17.36.39.mp4
I don't think that's a blocker for this PR as it's already an improvement over what's on trunk. It might be worth getting another approving review from folks who've also worked with the list view, as a confidence check just in case I'm missing additional context with the component, but it's looking good to me!
There might be a tiny bit of overlap with my WIP PR to add in multiple block selection in #38314 — but I'm happy to rebase / refactor that PR as needed once this one lands, if it gets in first 😀
ramonjd
force-pushed
the
try/expand-block-list-tree-on-selection
branch
from
1990cd7
to
4435683
Compare
|
Sorry to everyone that got pinged. Bad merge. Bad me. |
|
@andrewserong I've rebased on top of #38314. If you get time next week it'd be good if you could run your eye over things to make sure I didn't completely bork the rebase (again). |
|
Sure thing, thanks for updating this one @ramonjd! I'll give it a test on Monday 😀 |
Error: failed to find element matching selector ".block-editor-list-view-leaf:nth-child(2).is-selected a"
138 |
139 | // The block list should contain two leafs and the second should be selected (and be a paragraph).
> 140 | const selectedElementText = await page.$eval(
| ^
141 | '.block-editor-list-view-leaf:nth-child(2).is-selected a',
142 | ( element ) => element.innerText
143 | );The failing e2e is very strange. In the e2e test browser, I see the old markup, before we switched to a It was passing before the last rebase on top of trunk. e2e test
trunk/ this branch
Swapping the |
…t only when a block is selected outside the block list. Using an `useEffect` to loop through parent nodes, and expand them when necessary.
…ng in a new PR. Here we're calculating the top scroll position of the selected block.
…elected block *should* inform us that a block has been selected elsewhere, e.g., the editor. When we know this we can execute the hook logic that expands and scrolls to the selected element in the tree if a block is selected elsewhere. Removing hasFocus.
ramonjd
force-pushed
the
try/expand-block-list-tree-on-selection
branch
from
0c3eb7a
to
33cf482
Compare
It fixed itself :) |
andrewserong
approved these changes
Feb 21, 2022
There was a problem hiding this comment.
This is still testing nicely for me, @ramonjd! Thanks for rebasing and re-testing after the multi-select behaviour was introduced.
✅ Works well in post editor, site editor, and widget editor
✅ Multi-block selection behaviour is preserved
LGTM! Just left a tiny totally optional suggestion for a potential name change for the hook, but also fine to keep as-is if you prefer the current naming 🙂
|
Thank again for testing @andrewserong ! I'll let the tests run and merge this one. |
packages/block-editor/src/components/list-view/use-list-view-expand-selected-item.js
Outdated
Show resolved
Hide resolved
|
💯 |
cbravobernal
changed the title
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.
Description
This PR (maybe) resolves #34690
In this change, we expand the parents of nested items in the block list when the corresponding item is selected in the editor.
Oct-21-2021.21-14-34.mp4
Current limitations
Testing
Fire up the editor and insert several patterns with nested blocks, or create your own.
Example content
Open the List View tree and collapse a few nodes.
In the Editor, select an inner block. The tree should expand and reveal the nested block item.
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).