Site Editor: Make NavigationToggle filterable #33423
Conversation
Copons
added
[Type] Enhancement
Copons
requested review from
mattwiebe,
creativecoder,
david-szabo97,
jeyip and
Addison-Stavlo
|
Size Change: +38 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
Copons
changed the title
There was a problem hiding this comment.
What are the benefits of doing this via filter as opposed to a slot/fill like we did with the < Dashboard button for similar override capabilities?
IIRC when the < Dashboard button was extended the first thought was to use a filter similar to this, but it was advised to go with the slot/fill approach. I can't really remember the reasoning but maybe we should be considering the slot/fill here as well? 🤔
@Addison-Stavlo I tried with slot/fill first, but it quickly spun out of control, becoming massively more complicated and unnecessarily bloated. Some of the things that I've considered before getting to this PR:
I wonder if we decided to go with the slot/fill just for consistency with the post editor. 🤔 |
|
That all makes sense to me!
I can't recall if they were in slack channels prior to that PR, or even related to a previous PR that introduced that to the post editor? 🤔 I think the majority of that discussion may have been between @vindl and @youknowriad. The filter makes sense to me, but maybe @youknowriad could provide insight if there are any reasons to be concerned with this approach? 🙏 |
| @@ -83,4 +83,4 @@ function NavigationToggle( { icon, isOpen } ) { | |||
| ); | |||
| } | |||
|
|
|||
| export default NavigationToggle; | |||
| export default withFilters( 'editSite.NavigationToggle' )( NavigationToggle ); | |||
There was a problem hiding this comment.
For me it's not clear why we need this filter at all. and we've been moving away from withFilters and addFilter in Gutenberg. I know folks in WordPress want to be able to tweak everything in the UI but I don't think that's a healthy for the project in the long term.
There was a problem hiding this comment.
I don't think Slot and Fill is any better here personally, it's not clear what the slot would be, no semantics or anything. To be honest, I don't know what to do here.
There was a problem hiding this comment.
I know folks in WordPress want to be able to tweak everything in the UI but I don't think that's a healthy for the project in the long term.
I agree, it doesn't seem healthy. Adding tools to bypass UI because it isn't in the state we would like it to be, instead of taking the time to update the desired state seems unhealthy. Also both filter and slot/fill feel a bit odd on this toggle component.
Would a slot/fill make more sense on the panel content? If we want to abstract the template hierarchy away from new users, it would be beneficial to be able to remove the templates section from the navigation panel. I think it was with this idea that @Copons created a draft PR for adding a Slot/Fill for the actual sidebar content (#33298). To me, that makes a bit more sense for a slot/fill than this toggle button, but I wonder your thoughts on that as well? It would still be good to update the navigation panel in a way that is not overwhelming to new users while advanced features are still accessible for power users and theme developers, but if were running a platform where we are trying to handle the template hierarchy for the user then hiding that section altogether seems necessary.
There was a problem hiding this comment.
@Addison-Stavlo I think that I've figured out better ways to SlotFill that's more respectful of Core WP.
Basically, instead of SlotFill-ing the entire content, we should just simply add a SlotFill after it, so that folks can add other menus and whatnot.
If folks don't need the vanilla menus, they can probably just hide them via CSS for now.
This about the content would be a temporary stopgap anyway, while we work on improving the menus for everybody.
Im just realizing this still won't be a full solution even without considering plugins dispatching the relevant actions. As there are still other places in the interface that open the panel: I suppose this button could also be a problem if we were trying to replace the panel content / remove the templates section altogether. 🤔 |
|
I'll close this PR as it seems very misaligned with Core values. 😬 |
Description
Make the
NavigationTogglecomponent filterable, so that it's possible to override its props externally.Note
Practically speaking, in order to allow third parties to override the toggle's
onClick, this change will make them able to prevent opening the Navigation sidebar altogether.I haven't spent too much time thinking of a catch-all solution, as it's reasonably easy to restore the sidebar opening functionality in a plugin by dispatching the relevant actions.
If anyone has a better solution, though, feel free to share it! 🙂
How has this been tested?
packages/edit-site/src/plugins/index.js:NavigationTogglecomponent (the top-left WP or site icon).NavigationToggle.Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).