Persistent List View: Add visual support for multiple selected blocks #29878
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
Copons
added
the
[Feature] List View
Copons
requested review from
mattwiebe,
jameskoster,
vindl,
creativecoder,
david-szabo97,
jeyip and
Addison-Stavlo
|
Size Change: +59 B (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
jameskoster
requested changes
Mar 16, 2021
@jameskoster That's actually as I intended and mentioned in the PR description! 🙂
|
|
Ah, interesting. I think it's worth trying it with the highlight before ruling it out. I think it works ok in Figma as an example:
|
Copons
force-pushed
the
update/list-view-multiselect
branch
2 times, most recently
from
5dff276 to
fc7f61b
Compare
|
Once we have collapse / expand it might be worth collapsing on multi-selection |
mattwiebe
approved these changes
Mar 19, 2021
| @@ -74,7 +74,7 @@ export default function ListViewSidebar() { | |||
| <BlockNavigationTree | |||
| blocks={ rootBlocks } | |||
| selectBlock={ selectEditorBlock } | |||
| selectedBlockClientId={ selectedBlockClientId } | |||
| selectedBlockClientId={ selectedBlockClientIds } | |||
There was a problem hiding this comment.
Would we maybe want to update the param here and elsewhere to reflect its new, consistently plural state? Eg selectedBlockClientIds={ selectedBlockClientIds } ?
There was a problem hiding this comment.
Thanks for raising this, I was undecided if pursuing it, but in the end it wasn't that complicated.
In 51631bd I've replaced the singular prop with the plural.
Since the BlockNavigationTree component is exposed by the package, I added a deprecate message and a little prop conversion for backward compatibility.
(Note: the main BlockNavigation component is not exported, reason why I handled the deprecation in the sub-component BlockNavigationTree.)
I've also updated all the consumers of these components to the new array, plural prop.
As the multi-selection is currently only supposed to be supported by the persistent List View, I've chosen to keep the pre-existing behaviour for the other consumers (dropdown List View, and Navigation block's Block Navigation).
Namely: they only highlight singularly selected blocks, while multi-selected blocks are simply ignored.
The technical reason for this is that single and multiple selections refer to different state properties.
talldan
reviewed
Mar 22, 2021
Copons
force-pushed
the
update/list-view-multiselect
branch
from
5bbeb72 to
c89ff13
Compare
|
I've rebased this after #29902, which touched roughly the same files as this. I don't see any issues arising from the rebase, but would love a second look before merging this, as it will affect production components. |
david-szabo97
approved these changes
Mar 23, 2021
There was a problem hiding this comment.
🚢 🚀
Can you update the PR description to reflect the latest version?
For example PR description says:
Also, selected nested branches of multi-selected blocks won't be highlighted anymore.
This is a personal choice, as I feel it might add too much visual noise to the list.
And the screenshot is outdated.
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
Fix #29469
When selecting multiple blocks, the selected client IDs are stored in a separate state property than single selected blocks.
The non-persistent List View has no real reason to support multi-selected blocks, so it's ok for it to not displaying anything as selected when multiple blocks are selected.
On the other hand, the Persistent List View would look odd.
This PR adds visual support to the Persistent List View for multiple selected blocks.
This new feature is gated behind the
__experimentalPersistentListViewFeaturesflag to avoid conflicts with non-persistent uses of the List View.When multiple blocks are selected, opening the List View won't focus on the selected item anymore.
I'm not sure if focusing the first selected item would make more sense, though. 🤔
Note: this PR does not add support for selecting multiple block in the List View.
I'm not sure if it's possible, but I'd rather keep it simple for now.
How has this been tested?
Also check for regressions of the List View dropdown in the Post Editor.
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: