-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
[Site Editor - Page Inspector]: Add ability to switch templates #51477
Conversation
Size Change: +778 B (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
Flaky tests detected in 48ee0a9a59bcaa69af9891c3799fb20d3253ae66. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6148471084
|
Do you think we should be doing the same in the post editor, in the "template" panel as well. These seem like they should do the exact same thing? |
You mean the templates list, right? In post editor we have the same functionality with a select control in |
Yeah, my question essentially is why they have diverged, and should the swapping be similar in the post editor (with a visual list instead of a select) |
packages/edit-site/src/components/sidebar-edit-mode/page-panels/swap-template-button.js
Outdated
Show resolved
Hide resolved
} | ||
|
||
|
||
.edit-site-page-panels__swap-template__modal-content .block-editor-block-patterns-list { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm not wrong, this CSS is repeated in a lot of places, I wonder if we should have this in the component itself (like gridColumns prop or something like that)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true, but I focused for now with the technical implementation for the first round of reviews.
The post editor selector also works for classic themes, where a similar template list doesn't make sense, as we can't display previews for PHP templates. |
const { records: templates } = useEntityRecords( | ||
'postType', | ||
'wp_template', | ||
{ per_page: -1 } | ||
); | ||
const currentTemplate = useSelect( ( select ) => { | ||
const { getEditedPostType, getEditedPostId } = select( editSiteStore ); | ||
const { getEditedEntityRecord } = select( coreStore ); | ||
return getEditedEntityRecord( | ||
'postType', | ||
getEditedPostType(), | ||
getEditedPostId() | ||
); | ||
}, [] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion (non-blocking): The component only needs the data to check if there are more templates for selection. Maybe we should derive this data inside the mapSelect
and avoid passing templates
via props.
Mostly in the spirit of Redux and selecting minimal data required for a component :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean with inside mapSelect
?
|
||
function TemplatesList( { templates, currentTemplate, onSelect } ) { | ||
const { postId, postType } = useSelect( ( select ) => { | ||
const _context = select( editSiteStore ).getEditedPostContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question (unrelated): The selector looks to be deprecated (#46433) but is still used in a few places in the site editor. Should we un-deprecate it before there's a better alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. @youknowriad it seems even in your PR that deprecated, you still used it and there doesn't seem to be an alternative for that.. Any insights?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I probably deprecated it and changed my mind (too big of a refactor) and forgot about the deprecation.
Thanks @ntsekouras. For some reason I'm only seeing one template as an option, despite several custom templates existing. And when I swap to that template, I can no longer swap back 🤔 After swapping, the template is no longer listed in the details panel either. Quick video to demonstrate these issues: template.mp4A couple of nice-to-haves that can be addressed in follow-ups if need be:
Also leaving a note that the template panel itself could use some holistic design attention, and that we should circle back to that as part of the polish effort in the lead up to 6.3. |
One other observation: swapping the template seems to get saved instantly? I don't think that should be the case. |
I'm not sure how easy that would be because of:
That's the same problem the other way around.
Hm.. that's my logic's bug. I'll see to fix it. Write now I was fetching only the custom templates created by users. |
85e54f9
to
eae4759
Compare
829badb
to
13cfba0
Compare
That's the same with Chrome too.. We should pinpoint the problem though.. I'll cc @Mamaduka if has any ideas on top of his head.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments but nothing is blocking. The PR is very good code wise.
*/ | ||
import { store as editSiteStore } from '../../../store'; | ||
|
||
export function useEditedPostContext() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Do we really need this hook? Feels like a very light shortcut.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you're using this pattern for several hooks. Personally I wonder if this indirection is necessary to be honest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is going to increase the number of useSelect we have. While I didn't measure anything here. I wonder if a single useSelect is in general more performant than multiple ones.
All of this is still a small thing though. I'm fine if you think this makes the code clearer/better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mostly did this because of the different dependencies of other useSelect
in the code needed.
<MenuGroup> | ||
<MenuItem | ||
onClick={ async () => { | ||
entity.edit( { template: '' }, { undoIgnore: true } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we using undoIngore in these edits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the specific undo
needs a side effect(to update the edited entity).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that means we need to solve that to solve the undo as well #51477 (comment)
onClick={ async () => { | ||
entity.edit( { template: '' }, { undoIgnore: true } ); | ||
onClick(); | ||
await setPage( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea here to cause a refresh of the template or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly.
const template = await registry | ||
.resolveSelect( coreStore ) | ||
.__experimentalGetTemplateForLink( page.path ); | ||
|
||
if ( ! template ) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change in this action is fine, but what would be awesome (and probably better explored as a follow-up) is to remove the need for this action in favor of a selector + resolver
that contains the same logic.
In other words when the editor component would calls these selectors:
getEditedEntityId()
getEditedEntityType()
a resolver is triggered to actually "resolve" the template (so the logic we have right now in this action).
The reason for this is that we would be able to say that editEntityRecord
of the template
actually marks the resolver as "unresolved" which would cause the resolver to trigger again.
There are some subtle things to consider (loading...) that's why this is more of a quality of life improvement that should be addressed separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this is that we would be able to say that editEntityRecord of the template actually marks the resolver as "unresolved" which would cause the resolver to trigger again.
Can you expand on this a bit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that we can define the shouldInvalidate
property on the resolver to ensure that every time the edit entity action is called with a new template, the corresponding resolver is invalidated.
Regarding the TOC block issue, I think it's related to the specific block and doesn't work well if we have more headings in the template. I opened an issue for that.. |
Let's land this and follow up with improvements. Thank you all! |
It then got moved to the Summary panel for a couple of reasons:
I don't think any of that prohibits a revisit though, especially if upcoming enhancements require it. I believe the instant-save issue get fixed along the way. |
I don't want to block the work so happy to refine how we display the template in a follow up issue. |
This PR caused a regression with template lookup via the API. I created a patch here: #54599 |
Thanks for catching this and fixing this @mikejolley! 🚀 |
What?
Resolves: #51347
This is the first iteration to add the ability to switch templates in site editor's page inspector. Currently we show the
swap template
button if we have custom templates. Additionally we skip showing the button if we have only one custom template that is already assigned to the page.How?
For showing the templates list we use the
BlockPatternsList
component that handles many things already, like keyboard navigation, and for styling consistency with similar modal. Also in the future we could suggest patterns and create custom templates on the fly.The challenging thing for me was that while we need to edit one entity(
page
), we also need to update the current block editors' edited entity(template
). That's why I added thesetEditedPost
action, to update both at once. Furthermore theundo
functionality was added through thesnackbar
because the specificundo
needs a side effect(to update the edited entity). I'm open to suggestions for alternatives or something better.Testing Instructions
swap template
button the changes are applied and theundo
in snackbar works properly.Screenshots or screencast
Screen.Recording.2023-09-12.at.12.22.20.PM.mov