Wrap navigation editing features with filters #25329
Conversation
draganescu
requested review from
ajitbohra,
Soean and
talldan
as code owners
|
Size Change: -433 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
draganescu
changed the title
There was a problem hiding this comment.
This is pretty clever. I would personally be happy with this approach if it could be implemented in such a way that it didn't need a new navigation.BlockEdit filter, as that constitutes a very specific public API for a single block. In my quick testing it should be possible to use the existing editor.BlockEdit filter. I added a comment below on that.
Also noting the item justification isn't working for me in master, but in this PR it's fixed 😄
talldan
force-pushed
the
try/navigation-use-filter-add-controls
branch
from
19b1c11 to
899e406
Compare
|
@draganescu let me take over this PR, so I've made some changes:
Not sure if there are other options to remove. I wasn't sure about 'Anchor', is that in use? |
|
Anchor for the Navigation block doesn't make much sense so we can remove that in all the places. |
|
I've removed support for both anchor and additional class names on the nav screen for the nav block after a brief consult with @draganescu. |
| function removeNavigationBlockSettingsUnsupportedFeatures( settings, name ) { | ||
| if ( name !== 'core/navigation' ) { | ||
| return settings; | ||
| } | ||
|
|
||
| return { | ||
| ...settings, | ||
| supports: { | ||
| ...omit( settings.supports, [ | ||
| 'anchor', | ||
| 'customClassName', | ||
| '__experimentalColor', | ||
| '__experimentalFontSize', | ||
| ] ), | ||
| customClassName: false, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
I feel like an allowlist would be a better choice than a denylist here. With an allowlist, worst case scenario is not offering a new feature on this screen. With a denylist, worst case scenario is breaking the screen with unsupported feature that was added without considering this editor.
There was a problem hiding this comment.
I'm not sure that would be possible as block supports settings are an already defined public API.
With a denylist, worst case scenario is breaking the screen with unsupported feature that was added without considering this editor.
I'm not sure it'd break anything, it'd be a setting that would never have its value saved.
There was a problem hiding this comment.
I suppose the option would be to use a filter to add them to the post/site editor at registration, though I personally feel that blocks in the block library shouldn't require any extra configuration, they should work as intended out of the box.
| const removeNavigationBlockEditUnsupportedFeatures = createHigherOrderComponent( | ||
| ( BlockEdit ) => ( props ) => { | ||
| if ( props.name !== 'core/navigation' ) { | ||
| return <BlockEdit { ...props } />; | ||
| } | ||
|
|
||
| return ( | ||
| <BlockEdit | ||
| { ...props } | ||
| hasSubmenuIndicatorSetting={ false } | ||
| hasItemJustificationControls={ false } | ||
| hasListViewModal={ false } | ||
| /> | ||
| ); | ||
| }, | ||
| 'removeNavigationBlockEditUnsupportedFeatures' | ||
| ); |
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't agree as there are more cases where the setting needs to be true (post editor, site editor) than there are where it needs to be false (navigation editor). There are possibilities for mistakes either way.
Change navigation block to use __experimentalColor support option, and disable feature on navigation screen Remove unused ref Refactor to use feature flags injected as props to BlockEdit Also remove font size support Remove CSS Class and anchor support on navigation screen
talldan
force-pushed
the
try/navigation-use-filter-add-controls
branch
from
6834c5c to
e9670cc
Compare
Advances #25233
Fixes #25508
Description
This PR makes the Navigation block's edit component customizable for the Navigation editor. It adds all the formatting controls, the modal list view control, the alignment control in filters and removes them in the Navigation editor.
How has this been tested?
Tested locally.
Types of changes
Non breaking changes, but new pattern that may have some effects I am not aware of.
Checklist: