Navigation: Use the ListView in the Navigation block inspector controls #49417
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
scruffian
commented
Mar 28, 2023
scruffian
commented
Mar 28, 2023
packages/block-library/src/navigation/edit/menu-inspector-controls.js
Outdated
Show resolved
Hide resolved
scruffian
added
[Feature] List View
|
Size Change: +1.06 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
getdave
reviewed
Mar 29, 2023
getdave
reviewed
Mar 29, 2023
This was referenced Mar 29, 2023
|
@scruffian Let's try to get #49433 merged so as to provide more confident about this refactor. |
talldan
reviewed
Mar 30, 2023
packages/block-library/src/navigation/edit/menu-inspector-controls.js
Outdated
Show resolved
Hide resolved
|
This seems too easy 😄 |
|
I pulled out the extra prop into another PR to keep the changes as small as possible: #49475
We we did make other changes to ListView first to bring them inline :) |
getdave
force-pushed
the
use/list-view-in-inspector
branch
from
0539af0
to
13cae3c
Compare
scruffian
force-pushed
the
use/list-view-in-inspector
branch
from
13cae3c
to
f24e65d
Compare
scruffian
force-pushed
the
use/list-view-in-inspector
branch
from
c038273
to
77cc8e5
Compare
|
I added a commit to implement the |
ajlende
approved these changes
Apr 18, 2023
|
I think to get the tests to pass we also need to do this: #49907 |
aristath
changed the title
scruffian
force-pushed
the
use/list-view-in-inspector
branch
from
97376f8
to
40bcb98
Compare
|
Flaky tests detected in e73a0ae. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4929855278
|
scruffian
commented
May 10, 2023
| LeafMoreMenu={ LeafMoreMenu } | ||
| description={ description } | ||
| /> | ||
| <div className="wp-block-navigation__menu-inspector-controls"> |
There was a problem hiding this comment.
Can we avoid this extra div?
There was a problem hiding this comment.
no, we need the classname on a div, not the table element for the horizontal scroll to work
draganescu
previously approved these changes
May 11, 2023
There was a problem hiding this comment.
This tests well for me and the code does the job of making the leafmoremenu a component that is composed from the various places that use ListView.
The leaf more menu in the navigation screen of the site editor does not perfectly work. Particularly the add submenu:
- does not expand the link with the added submenu item
- the added submenu item shows up LinkUI - which is as broken as always in this context
I will approve this anyway as I think it's a good step forward.
draganescu
reviewed
May 11, 2023
| ref | ||
| ) { | ||
| // This can be removed once we no longer need to support the blocks prop. | ||
| if ( blocks ) { |
There was a problem hiding this comment.
Are we sure we want to remove this? @talldan doesn't this remain a viable alternative, giving ListView a list of blocks? I think we're corneting ListView intro a place where it can only work along with an instanced block editor tree. I am suspicious if this actually improves ListView or takes away.
I'd be happy in the future to have some need to pass a tree of blocks and for it to work.
There was a problem hiding this comment.
I'm not sure when blocks was introduced, but it is a little open to issues:
- Some selectors that return block trees only contain uncontrolled blocks
- ListView is optimized to work with a
clientIdsTreerather than a full block tree, passing in a full block tree is overly expensive.
I think we're corneting ListView intro a place where it can only work along with an instanced block editor tree.
It's a bit of an existential issue. I'm not sure what use cases there would be for it work with a non-instanced block editor.
ajlende
reviewed
May 11, 2023
|
|
||
| export default forwardRef( ( props, ref ) => { | ||
| return ( | ||
| <PrivateListView | ||
| ref={ ref } | ||
| { ...props } | ||
| showAppender={ false } | ||
| blockSettingsMenu={ BlockSettingsDropdown } |
There was a problem hiding this comment.
I removed this in my commit because BlockSettingsDropdown is the default value, and it meant that there were fewer things that needed to be imported here.
This change does mean that it can be overridden by props passed to ListView now, so if that shouldn't be possible, then it needs to be added back as undefined or something.
There was a problem hiding this comment.
Yeah we can't do that because the point it to restrict this from being prop being used by third parties. undefined should work though, I'll push that now.
scruffian
force-pushed
the
use/list-view-in-inspector
branch
from
b892f51
to
7555b87
Compare
| * @param {Function} props.renderAdditionalBlockUI Function that renders additional block content UI. | ||
| * @param {Ref} ref Forwarded ref | ||
| */ | ||
| const PrivateListView = forwardRef( function ListViewComponent( |
There was a problem hiding this comment.
Apologies if this has already been discussed somewhere, but I'm wondering why this component is being moved to another file? It creates quite a big diff, which means that we could lose some useful history in the ListViewComponent. Is it possible to keep ListViewComponent in index.js?
There was a problem hiding this comment.
We didn't want to move it, but webpack was causing a build error with having both exports in the index.js file. It was working fine, then all of a sudden was not working on anyone's machine. It seemed to be a build issue and not related to the code itself, and moving the file fixed it. At least, that's my understanding from @ajlende. He may be able to provide more context.
There was a problem hiding this comment.
I moved it back in 3fe3bf9
scruffian
force-pushed
the
use/list-view-in-inspector
branch
2 times, most recently
from
cd8f581
to
f61c627
Compare
use the hook from list view remove prop drilling remove prop drilling Allow list view to scroll add custom scrollbars on hover update navigation block tests to account for using the list view fix test List View: Add a new prop to allow blocks in the inserter to be prioritized add another prop to list view Don't bother sorting if there are no proitizedInnerBlocks Open LinkUI popover after appending link from Navigation side block inserter The behavior when adding a link from the appender button on the OffCanvasEditor is to open a link control popover. This commit copies over several files from the OffCanvasEditor that was required for that to work in the ListView. The behavior is buggy, IMO, but it does match what is in trunk. inverted colors for the scrollbar Moved files related to navigation block outside of list-view. Remove unused link-ui from list-view Fix webpack runtime error For some reason keeping both exports in the same file caused an error that PrivateListView was used before initialization remove unneeded code to prioritize the blocks in the inserter remove unused prop move the inline function to a separate definition remove unneccessary change
draganescu
force-pushed
the
use/list-view-in-inspector
branch
from
02564c5
to
c23b638
Compare
|
Nice work landing this one, folks! 🎉 |
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?
This replaces the OffCanvasEditor with the ListView component in the inspector controls for the Navigation block
Why?
We shouldn't have two components that do the same thing.
How?
Implements the new props we have added.
Also adds a wrapping div to allow the list view to scroll horizontally.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Horizontal scrollbar:
Closes #49986