Site Editor: Add template switcher to navigation panel #25615
Conversation
david-szabo97
added
[Feature] Full Site Editing
[Feature] Navigation Component
|
|
Size Change: -56 B (0%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
| ) } | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
This looks very similar to NavigationItemWithIcon, could we "merge" the 2 components?
There was a problem hiding this comment.
Kinda different 🤔 I unified them but I'm not sure if it's any better this way. Both way seems a little bit confusing. Maybe it's too smart now.
packages/edit-site/src/components/left-sidebar/navigation-panel/template-switcher/index.js
Outdated
Show resolved
Hide resolved
| { icon && ( | ||
| <div className="edit-site-template-switcher__label-home-icon"> | ||
| <Icon icon={ icon } /> | ||
| </div> |
There was a problem hiding this comment.
TemplateLabel wrapped the icon in Tooltip.
I think it's a nice and easy a11y win if we brought the same feature here.
We could add an iconLabel prop for the tooltip, that would default to title if not provided.
packages/edit-site/src/components/left-sidebar/navigation-panel/template-switcher/index.js
Outdated
Show resolved
Hide resolved
| ); | ||
| const blocks = useMemo( | ||
| () => ( template ? parse( template?.content?.raw || '' ) : [] ), | ||
| [ template ] |
There was a problem hiding this comment.
template is an object, are we sure this useMemo is only updating when template changes instead of every render?
Same goes for the item dependency of the useSelect above.
Do you think it would make more sense to destructure the item ID and use that as dependency?
e.g.
function Template Preview( { item } ) {
const { id: itemId } = item;
const template = useSelect( () => {}, [ itemId ] );
const blocks = useMemo( () => {}, [ itemId ] );
}There was a problem hiding this comment.
How about using template?.content?.raw || '' as a dependency? Probably itemId is safe too.
...edit-site/src/components/left-sidebar/navigation-panel/template-switcher/template-preview.js
Outdated
Show resolved
Hide resolved
| /> | ||
| <div className="edit-site-template-switcher__theme-preview-description"> | ||
| { truncate( | ||
| /* Not using description.rendered here, as we might contain after an opening HTML tag. */ |
There was a problem hiding this comment.
We should probably clarify this comment 😄
...edit-site/src/components/left-sidebar/navigation-panel/template-switcher/template-preview.js
Outdated
Show resolved
Hide resolved
...es/edit-site/src/components/left-sidebar/navigation-panel/template-switcher/theme-preview.js
Outdated
Show resolved
Hide resolved
| label: <TemplateLabel template={ templatePart } />, | ||
| value: templatePart.id, | ||
| slug: templatePart.slug, | ||
| } ) ); |
There was a problem hiding this comment.
We probably don't need this map here, as we can just as easily do it once in the render below.
E.g.
{ templateParts?.map( ( templatePart ) => (
<NavigationItem
key={ `template-part-${ templatePart.id }` }
item={ templatePart.slug }
title={ <TemplateLabel template={ templatePart } /> }
onClick={ () => onActiveTemplatePartIdChange( templatePart.id ) }
onMouseEnter={ () => onHoverTemplatePart( templatePart.id ) }
onMouseLeave={ () => onHoverTemplatePart( null ) }
/>
) ) }Note also that while there are no checks, title is supposed to be a string.
I guess that if we unify TemplateLabel and NavigationItemWithIcon this inconsistency wouldn't happen anymore.
There was a problem hiding this comment.
🤔 If we are supposed to use string as title, then I need to copy-paste the whole part from the NavigationItem to get the same styling with a custom icon.
|
@david-szabo97 I totally failed to convey what I was thinking for the return (
<NavigationItem item={ item } title={ title }>
<Button { ...props }>
{ title }
{ icon && (
<Tooltip text={ iconLabel || title }>
<div className="edit-site-template-switcher__label-home-icon">
<Icon icon={ icon } />
</div>
</Tooltip>
) }
</Button>
</NavigationItem>
);Note that |
Yeah I guess there shouldn't be any harm here in removing the header's
🤔 Do we have a case of multiple active items? If we need multiple active items, we should have multiple |
| activeItem={ | ||
| templateType === 'wp_template_part' && | ||
| `template-part-${ templatePartId }` | ||
| } |
There was a problem hiding this comment.
How do you feel if we use this to also also activate the template?
If we add the template ID as item to its own NavigationItem (e.g. item={ 'template-' + templateId }), then here we could do something like this:
<Navigation activeItem={ 'wp_template' === templateType
? `template-${ templateId }`
: `template-part-${ templatePartId }`
} />
I was thinking about how we are going to put the template switcher and the page switcher on the same navigation. I guess they are going to be nested so we won't have multiple active items. |
No need to keep the source, we can always bring it back from git history if needed.
We don't have to worry about pages for now, just templates. |
david-szabo97
changed the title
There was a problem hiding this comment.
This tests well for me but we'll need to update the failing e2e tests to account for template switcher changes. We also need to display all templates and not just the ones related to current page content selection, but that can also be done in a follow-up PR.
david-szabo97
requested review from
ajitbohra,
nerrad and
ntwb
as code owners
david-szabo97
force-pushed
the
add/navigation-panel-template-switcher
branch
from
d0fa016
to
ec18fc1
Compare
|
Rebased because of #25622. It added a slide-in animation to the navigation which seems to also occur when we are switching between template and template part 🤔 We are merging this and we will fix the animation in a separate PR. |
Related: #25262
Notes about code
template-switcheris a copy of the originaltemplate-switcherfound in the header.template-switcher/index.jsis overhauled to work with theNavigationPanelChanges intemplate-switcher/template-preview.jsandtemplate-switcher/theme-preview.jsare minimal. Only changed the container's (very-first-div)className. Feel free to ignore these during reviewtemplate-switcheroriginal styles are used and added some overrides in thenavigation-panel/styles.scssDescription
This change replicates the template 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: