Skip to content

Site Editor: Make NavigationToggle filterable #33423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Copons wants to merge 1 commit into from
Closed

Site Editor: Make NavigationToggle filterable #33423

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/edit-site/src/components/navigation-sidebar/navigation-toggle/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { useDispatch, useSelect } from '@wordpress/data';
import { Button, Icon } from '@wordpress/components';
import { Button, Icon, withFilters } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { wordpress } from '@wordpress/icons';
import { store as coreDataStore } from '@wordpress/core-data';
Expand All @@ -12,7 +12,7 @@ import { store as coreDataStore } from '@wordpress/core-data';
*/
import { store as editSiteStore } from '../../../store';

function NavigationToggle( { icon, isOpen } ) {
function NavigationToggle( { icon, isOpen, label, onClick } ) {
const {
isRequestingSiteIcon,
navigationPanelMenu,
Expand Down Expand Up @@ -73,8 +73,8 @@ function NavigationToggle( { icon, isOpen } ) {
>
<Button
className="edit-site-navigation-toggle__button has-icon"
label={ __( 'Toggle navigation' ) }
onClick={ toggleNavigationPanel }
label={ label || __( 'Toggle navigation' ) }
onClick={ onClick || toggleNavigationPanel }
showTooltip
>
{ buttonIcon }
Expand All @@ -83,4 +83,4 @@ function NavigationToggle( { icon, isOpen } ) {
);
}

export default NavigationToggle;
export default withFilters( 'editSite.NavigationToggle' )( NavigationToggle );
Copy link
Contributor

@youknowriad youknowriad Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@youknowriad youknowriad Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Copons Copons Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.