-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Edit Site: New template creation flow. #22586
Conversation
de9a3f6
to
782f2b6
Compare
Size Change: +56 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
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.
Really cool stuff here! Not really any major concerns from me. As we've been mentioning, moving a lot of this logic into a store would make the code cleaner and separate "business logic" from the UI components a bit better. But that's a separate task. I also imagine the UI for this will eventually change, so I'm not all that concerned about how it looks or its UX. I think the main thing is adding a way to override a template in the first place
/** | ||
* WordPress dependencies | ||
*/ | ||
import { useSelect, useDispatch } from '@wordpress/data'; |
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.
Interesting, so instead of creating an arbitrary template, we would now have to override the base template level-by-level on a certain page type until we get to the desired specificity? I imagine we'll want to make it easier to "zoom in really far" in one go in the future
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.
There are only two levels of specificity:
- Page type, e.g., category, page, post.
- Page-specific, e.g., a specific category, specific page, specific post.
@@ -33,8 +33,13 @@ export default function PageSwitcher( { | |||
const path = getPathFromLink( _page.link ); | |||
return { | |||
label: _page.title.rendered, | |||
type: 'page', | |||
slug: _page.slug, |
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 this the template slug?
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 page slug.
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 realized that's what the variable is called, but was originally wondering if it had to do with the page page instead of the page cpt
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.
Well, the page page's slug is the same as the page CPT slug in this case. It's what we need to generate its specific template slug.
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.
Ah, that makes sense
import TemplatePreview from './template-preview'; | ||
import ThemePreview from './theme-preview'; | ||
|
||
const TEMPLATE_OVERRIDES = { |
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's the path forward for supporting more of the possible overrides in the template hierarchy? Will we just add more keys here, or make it more dynamic?
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.
Both are viable. It depends on what templates we need to support.
} ); | ||
onAddTemplateId( newTemplate.id ); | ||
}; | ||
const unoverwriteTemplate = async () => { |
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.
We should probably prompt the user for confirmation before deleting a template
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.
Yeah, I think we need to work it into the "Review Changes" sidebar somehow.
@@ -175,15 +202,6 @@ export default function TemplateSwitcher( { | |||
</> | |||
) } | |||
</DropdownMenu> | |||
<AddTemplate |
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.
removing this will definitely break the dirty state entity e2e tests. Not sure what the best path forward is for those e2e tests considering that the UI is likely to continue changing a lot.
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.
Can we update them to use the API directly?
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.
🤔 Maybe? Like the WP API or the block-editor API? I feel like the e2e test ought to be going through real user actions at the end of the day. Otherwise, the unit tests on useBlockSync
would be good enough, right?
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.
Yeah, I agree. It's just that the UI will probably change a lot with the new designs coming up. Using the WP API, for now, would let us have some sort of safety net, without having to have significant test rewrites as we iterate.
That said, we don't have that many tests right now, I think it makes sense to update them in this PR.
}; | ||
const unoverwriteTemplate = async () => { | ||
await apiFetch( { | ||
path: `/wp/v2/templates/${ template.value }`, |
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.
Should the core/data controls handle this?
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.
Yeah
ce54adb
to
1814e0e
Compare
@noahtallen Can you take a look at why the tests are failing, please? It's not even opening the inserter. |
@noahtallen Fixed them! This is ready for review 🚀 🚀 🚀 |
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.
It's pretty incredible how flexible the entity system is. In the site editor I can open a category page showing a list of posts and update the post content of a post right from there. I can even update reusable blocks inside of post content. It's pretty cool that it works!
Overall, I think this is an improvement in that it forces you to only create templates that will actually work with your site's pages. (So you can't create dangling templates that you won't see any more.)
On the other hand, I found it difficult to navigate back to the "home page" or index template, because at first I didn't know which entity to select to see it. how easy would it be to add a "home" icon to the current homepage?
One final thing I noticed is this bug, where you can't select "All posts" after selecting "sample page":
That console error logs every time I click "all posts".
Additionally, I noticed the whole editor screen flashes white when you navigate to "sample page" at first. Is something causing the editor component to remount?
Yes, the entire block list is changing. I don't see white, though; I see the editor's background color. |
Interesting. I wasn't really expecting the toolbar and stuff to disappear when that happened. Nice home icon. Gets the job done for now! |
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.
Approving with the expectation that designs will improve in the future and that we'll be able to clean up the code a bit more by integrating it to the edit-site store
9ffec57
to
72de844
Compare
The failed tests are unrelated. I think they are caused by an |
Description
This PR continues the work to make page template navigation in the editor page-centric.
#22368, #22449, and #22543 implemented functionality for navigating between pages while resolving and loading their templates. The current template switcher that lets you select from any of your templates doesn't make a lot of sense anymore.
This PR modifies the template switcher to better align with the new page-centric navigation. Now it will only show the currently resolved template and the option to overwrite it for the current page or to revert overwrites.
This means that you can have a category template for all your categories, a page template for all your pages, a post template for all your posts, and then spin up variations for specific categories, pages, or posts, with the click of a button. Then, you can revert just as easily. The GIF below demonstrates the process for a category.
Testing this PR required changing between lots of templates, and this helped catch some related bugs that were solved with a bit of hardening:
lib/compat.php
hook that checks for category queries now works for both category slugs and IDs.useBlockSync
hook now guards against potential race conditions:How has this been tested?
It was verified that the flow shown in the GIF works as expected and that nothing breaks or is cleared when switching between templates.
Screenshots
Types of Changes
New Feature: The site editor's navigation continues to improve, and now supports spinning up variations of the current template for the current page.
Checklist: