Site Editor: Move page switcher to navigation panel #25620
Conversation
david-szabo97
added
[Feature] Full Site Editing
[Feature] Navigation Component
|
Size Change: -4.78 kB (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
It looks like we won't be showing content selection options in the sidebar for now, only the templates and template parts. The actual content to be viewed or edited might be provided through Preview or in some other way. Taking that into account I think we should close this out for now, and reopen it later if needed.
The time has come. 😄 @david-szabo97 @Copons I think we should place this in a separate submenu |
david-szabo97
force-pushed
the
add/navigation-panel-page-switcher
branch
from
1fd55ff to
f4c819f
Compare
david-szabo97
changed the title
There was a problem hiding this comment.
This is shaping up nicely! ✨
It works as expected, but I've left a few comments about the code organization, as I think we should be able to simplify the content menu a little bit.
It's a bit of a bummer that we are losing LinkControl (the search post function), but it seems more appropriate to take care of it in a follow up, rather than cluttering this PR with a possibly large amount of CSS changes. 🙂
packages/edit-site/src/components/left-sidebar/navigation-panel/menus/content.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/left-sidebar/navigation-panel/navigation-entity-items.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/left-sidebar/navigation-panel/index.js
Outdated
Show resolved
Hide resolved
| onClick={ () => onPageSelect( entity, item ) } | ||
| /> | ||
| ) } | ||
| /> |
There was a problem hiding this comment.
By now you might have noticed that I'm not very into render props. 😄
In this specific case I don't really see the need for it.
How about just passing kind and name (and query), and let the NavigationEntityItems do all the heavy lifting?
E.g.
function NavigationEntityItems( { kind, name, query } ) {
const entities = useSelect( /* ... */ );
const { setPage } = useDispatch( 'core/edit-site' );
if ( ! entities ) {
return;
}
const onSelectItem = ( { type, slug, link, id }, item ) => {
setPage( { /* ... */ } );
};
return ( <Fragment> { entities.map( ( entity ) => {
const key = `content-${ kind }-${ name }-${ entity.slug }`;
return ( <NavigationItem
key={ key }
item={ key }
title={ entity.title.rendered }
onClick={ () => onSelectItem( entity, key ) }
/> );
} ) } </Fragment> );
}| settings={ {} } | ||
| noDirectEntry | ||
| showInitialSuggestions | ||
| /> |
There was a problem hiding this comment.
I'd have loved to have this in the sidebar as well, but its style is very opinionated, and I guess it would require a beefy effort.
Let's make sure we follow up on this though!
|
|
||
| const { | ||
| setTemplate, | ||
| setTemplatePart, | ||
| setNavigationPanelActiveMenu, | ||
| setPage, |
There was a problem hiding this comment.
I've just realized this, and I've implicitly mentioned it in another comment, but I kinda feel like we could move some of these actions closer to where they are used, because, I might be wrong, it doesn't look like they are needed here.
So, setPage could live directly in the content menu (content.js).
setTemplate and setTemplatePart could be moved in their corresponding menus (template.js and template-part.js), or even both into template-navigation-items.js (using entityType to determine which to use).
(Note: if we agree on this move, we can take care of template and template part elsewhere)
There was a problem hiding this comment.
This sounds good to me! Probably template-navigation-items.js is the best place for it. It will make the component pretty smart though 😬
| const { show_on_front: _showOnFront } = getEditedEntityRecord( | ||
| 'root', | ||
| 'site' | ||
| ); |
There was a problem hiding this comment.
I've just noticed that we are losing all the "show on front" logic from the page switcher. 🤔
gutenberg/packages/edit-site/src/components/page-switcher/index.js
Lines 69 to 88 in c120818
Am I missing something?
There was a problem hiding this comment.
Oooops 😄 Pushed a commit which re-adds it.
packages/edit-site/src/components/left-sidebar/navigation-panel/index.js
Outdated
Show resolved
Hide resolved
|
After some thought, I think it would be better if we exposed the Content as a separate navigation in the top level, similar to Theme now. In it we could expose Pages, Posts, and Categories as separate submenus. Something like this:
|
I don't have a strong opinion either way, but that should be fairly easy by simply adding a separate |
I have a preference for it. The content is independent of the theme, so it doesn't make much sense to nest content under it. It allows us to get to pages and posts with one less click. We don't get into lack of space issues when trying to show them all in one menu (or at least not as early), and the search will probably be better contained. |
| @@ -3,6 +3,7 @@ | |||
| height: 100%; | |||
| position: relative; | |||
| width: $nav-sidebar-width; | |||
| background: #1e1e1e; | |||
There was a problem hiding this comment.
It would be great to put this somewhere as a variable. Like we did with nav-sidebar-width. Which file would be the place for it?
There was a problem hiding this comment.
Don't we already have a base variable for this in darkGray.primary and $gray-900?
There was a problem hiding this comment.
We do, but if we change the Navigation component's background color, then we need to change it everywhere else. That's why a variable would be handy. This background should match the Navigation's component background all the time.
There was a problem hiding this comment.
Good point, but I'm not sure we have an established way to do it. 🤔
I'd say let's go with $gray-900 here for now, and maybe we can circle back in the future to have a single variable for all sidebar backgrounds.
| import ContentPostsMenu from './menus/content-posts'; | ||
|
|
||
| export default function ContentNavigation() { | ||
| const [ activeMenu, setActiveMenu ] = useState( 'root' ); |
There was a problem hiding this comment.
Having two Navigation components make things interesting. Should we store this in the store as well?
There was a problem hiding this comment.
cc @Copons on this since he was advertising the ability to have multiple navigations in one view 😄
There was a problem hiding this comment.
If it's working fine for now I'd defer the store migration for a separate PR.
There was a problem hiding this comment.
advertising
😆
Well, I mean, the Navigation component is one part of the sidebar, so there are literally no reasons why a sidebar can't have more than one!
Also, IIRC the component's storybook used to have multiple Navigation in the same example (but this might have been just some WIP code on my machine, never released to the general public).
There was a problem hiding this comment.
About the migration to Redux: let's do it in a follow up once #26003 is merged, so that we can nest it inside navigationPanel.
I think this is fine for the first pass. As far as I'm concerned we can merge this once the tests are fixed. |
david-szabo97
force-pushed
the
add/navigation-panel-page-switcher
branch
from
eafc013 to
137340b
Compare
There was a problem hiding this comment.
Looking good @david-szabo97!
I've left a few requests, but they should be very straightforward!
packages/edit-site/src/components/left-sidebar/navigation-panel/content-navigation.js
Outdated
Show resolved
Hide resolved
| @@ -3,6 +3,7 @@ | |||
| height: 100%; | |||
| position: relative; | |||
| width: $nav-sidebar-width; | |||
| background: #1e1e1e; | |||
There was a problem hiding this comment.
Good point, but I'm not sure we have an established way to do it. 🤔
I'd say let's go with $gray-900 here for now, and maybe we can circle back in the future to have a single variable for all sidebar backgrounds.
packages/edit-site/src/components/left-sidebar/navigation-panel/navigation-entity-items.js
Outdated
Show resolved
Hide resolved
|
I tested this a bit more with the latest update. One unexpected consequence of introducing the Content like this is that two Navigation menus are independent now. Content will remain in view while navigating templates. It's also awkwardly changing its position all the time depending on the number of items in the menu above. I think it would be better to hide the Content menu once we drill down into Templates and vice versa, but retain the way it looks now if that makes sense. What do you all think? |
david-szabo97
force-pushed
the
add/navigation-panel-page-switcher
branch
from
137340b to
45e658c
Compare
|
Made the changes requested by @Copons and
is done as well |
|
Awesome ✨ this looks good to me now. 👍 |
Move page selection dropdown from the header to the navigation sidebar.
Related: #25262 #25255
Description
This change replicates the page switcher from the header in the Navigation sidebar.
How has this been tested?
yarn wp-env startScreenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: