Post Editor: Persistent list view#31047
Conversation
|
Size Change: +928 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This looks good so far, although im not sure about the failing e2e's. Works as expected in the post editor! 🎉
A bit of a tangent / unrelated to introducing this to the post editor specifically: The more I play with the persistent list view, the more I am upset that the block inserter closes it indefinitely. That is, I would expect if I have the list view open and then open the block inserter, that after the block inserter closes the list view is open again. It seems a bit mildly frustrating to have to re-open the list view every time I use the block inserter if my goal is to edit with the aid of the persistent list view.
@Addison-Stavlo failing e2e are definitely relevant, and I'll take care of them soon™.
This is tracked separately in #29468 |
priethor
left a comment
There was a problem hiding this comment.
Thanks for tackling this, @Copons! So far it looks great, both inserting blocks:
Apr-21-2021.18-12-09.mp4
And selecting/hovering blocks:
Apr-21-2021.18-17-18.mp4
I also noticed an inconsistency between the inserter and the list view, not directly related to this PR, which has to do with the toggle buttons that open both panels: the inserter button toggles between the "+" and an "X", whereas the List View one doesn't. However, the list view displays a "X" inside its panel that the inserter doesn't.
Apr-21-2021.18-17-43.mp4
Should the list view close button look as in the inserter for better consistency?
@priethor Good catch! The implementation will be slightly different though: the Inserter button just simply rotate the "+" icon by 45 degrees, whereas for the List View we'll need to completely replace the icon and lose the animation. |
|
My thinking is that the Inserter is a unique case in this regard, and that we shouldn't be swapping the icons for other toggles. If we did this for list view, we'd also need to do it for the settings/cog icon on the right-hand side of the top bar (and things like global styles). With multiple |
|
Also, if we move forward with #29468 and allow the inserter and the list view to be open at the same time, it makes a lot of sense to leave the close icon as it is; otherwise, there could be two "X" icons at the same time in the top left menu and it would become harder to know which closes what. |
|
The e2e tests are still running, but I expect one of them to still fail because of a tricky issue affecting the List View itself. All the info and the fix: #31058 |
packages/edit-post/src/components/secondary-sidebar/inserter-sidebar.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
| > | ||
| <BlockNavigationTree | ||
| blocks={ clientIdsTree } | ||
| selectBlock={ selectEditorBlock } |
There was a problem hiding this comment.
Why are these props not something handled inside BlockNavigationTree directly: blocks, selectBlock, selectedBlockClientsIs ?
There was a problem hiding this comment.
Because BlockNavigationTree would "normally" be called by BlockNavigation, which provides those props:
The major difference is that BlockNavigation only display the blocks for the currently selected hierarchy (e.g. if you are in a template part, it limits the blocks to those contained in that template part), whereas the persistent list view wants to display all of them.
When I wrote the persistent list view for the Site Editor, I intentionally decided to not touch BlockNavigation to avoid affecting the other consumers of the component.
But still, looking at how the list view is used, I can see that most consumers (edit-post in this PR, edit-site, and the Navigation block) use BlockNavigationTree, providing their own props.
Only edit-widgets is still using BlockNavigation through the dropdown.
It might be worth it to refactor BlockNavigation to reflect this shift, but at the same time, while all sub-components (such as BlockNavigationTree) are marked as experimental, BlockNavigationDropdown (which uses the main BlockNavigation) is exported as stable, and so we would need a deprecation path for this.
There was a problem hiding this comment.
Looking at the code, it seems the only difference in BlockNavigation is that it only shows the selected block's tree if it's a container. I'm fine moving that logic into BlockNavigationTree with just a flag prop.
There was a problem hiding this comment.
Since it affects very different areas, I'd do it in a separate PR though, either before or after merging this.
8a5ca8d to
fc72339
Compare
eb44e06 to
d247c9a
Compare
cb6beec to
89a2947
Compare
ntsekouras
left a comment
There was a problem hiding this comment.
I have tested the PR and things seem to work as expected 👍 . Let's rebase as there are some conflicts.
I think the a good concern is this one by Riad: #31047 (comment), but has been discussed for a follow up.
| <ToolbarItem | ||
| as={ BlockNavigationDropdown } | ||
| isDisabled={ isTextModeEnabled } | ||
| as={ Button } |
There was a problem hiding this comment.
Just curious here if we should use ToolbarButton here now instead of ToolbarItem. --cc @diegohaz
There was a problem hiding this comment.
Oh, interesting!
Is there a reason why all the HeaderToolbar items are still ToolbarItem? At a quick glance it seems they could easily be ToolbarButton instead.
packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
Show resolved
Hide resolved
packages/edit-post/src/components/secondary-sidebar/list-view-sidebar.js
Show resolved
Hide resolved
@ntsekouras This is actually being addressed in #31290 🙂 That refactor ended up being simple enough that imho it makes more sense to wait for that before continuing this. Your review touches unrelated stuff, so I might as well address your suggestions now anyway. ✨ |
85b3603 to
30d91af
Compare
30d91af to
0e4e6bb
Compare
ntsekouras
left a comment
There was a problem hiding this comment.
LGTM 👍 - thanks @Copons!
With green CI feel free to land and we can focus on your follow up for the simplification of BlockNavigation component.
|
@ntsekouras and @Copons I just tested RC7 and I just want to say THANK YOU at this point. This is so fun with the optimization of the navigator. I work with it every day and this PR has improved the function significantly. I only miss down and up arrows to change the blocks-position in the navigator as well. And maybe at some point you could name blocks in the navigator? Thank YOU!! |
❤️ Arrows and drag and drop are on our radar already (#29729), but feel free to open an issue for renaming blocks straight in the list view! 🙂 |
|
Great to hear! Beyond the arrows and drag and drop you can follow up on the next planned tasks for this view in #29733. |
|
@SchneiderSam it's not quite naming blocks, but if you use anchors they now show up in the list view too:
|
Description
Fixes #29470
Fixes #22113
In the Post Editor, convert the List View dropdown into a persistent sidebar occupying the same interface skeleton slot as the Inserter (
secondarySidebar), in the same way it has already been done for the Site Editor.To achieve this, a number of widespread changes have been made:
components/secondary-sidebarfolder containing both the Inserter and the List View file, making the Post Editor's layout easier to read.listViewPanelreducer (and relative action and selector) in the same vein as the one for the inserter panel. These reducers are interconnected: only one can be open at any time, so the action that opens one will automatically close the other.BlockNavigationDropdownin the Post Editor header with a custom implementation that would fill thesecondarySidebarwith theBlockNavigationTreecomponent, displaying the entire blocks hierarchy of the content.BlockNavigationTreeitself).Note
We currently don't have a good way to focus back to the List View via keyboard, beside toggling it off and on again.
The issue is known and tracked in #29466.
Any suggestion would be extremely welcome!
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).