-
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
Implement "Add New" for templates list in Site Editor #36592
Conversation
Size Change: +650 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
1d746ab
to
28e7eb3
Compare
d5a3fa8
to
256a1ef
Compare
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.
Looks pretty good, nice work here. It's great you were able to refactor a couple of things and fix a bug along the way.
I think it should be possible to smooth out the experience when creating items, the way the table updates in the background briefly before navigating away is a little jarring at the moment.
Loading states, snackbars and error handling are also things to consider. But it's also ok to divide these things up into a separate task IMO.
packages/edit-site/src/components/create-template-part-modal/index.js
Outdated
Show resolved
Hide resolved
<div> | ||
<Button variant="primary">{ postType.labels?.add_new }</Button> | ||
<div className="edit-site-list-header__right"> | ||
<AddNewTemplate templateType={ templateType } /> |
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.
Not something to action, but just thinking about the future. Later on I wonder if we can consider an architecture like this:
<EntityListing type="postType" name="template">
<HeaderActions>
<AddNewTemplate />
</HeaderActions>
</EntityListing>
<EntityListing type="postType" name="template-part">
<HeaderActions>
<AddNewTemplatePart />
</HeaderActions>
</EntityListing>
This way there isn't an individual component that manages so much complexity for each different type.
Also thinking about how we might potentially add more entity types here in the future (menus?), and that this might also end up being useful for posts or pages too. Possibly the start of reactifying some of WordPress admin.
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.
Looks interesting! Though I believe we don't need a fancy name Like EntityListing
for that, to me it's just a Context.Provider
😅 .
It's also not ideal to render them all even if they won't be rendered in the end, the hooks will still run and potentially other side effects. I think we can re-think this in #36488 and use a route system for that. They are, just search params in the end.
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 way hooks execute is more of an implementation detail. The code example is just pseudocode.
The point I'm trying to make is that in the future the architecture can support some level of declarative customisation.
At the moment the solution is to add more implicit if
statements in components like AddNewTemplate
, but that should instead be inverted so that the control is in the hands of the implementor of these list pages. Imagine it from the point of view of this being a library of components for a user that might want to build their own list of posts, but with custom 'add' buttons, custom columns in the table.
All food for thought for after 5.9 though, but I think there's a good chance that will be needed—initially for menus, but the future might hold more.
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.
100% agree! I also agree that it's a bit early to overthink these at the time. There's something we can do now though, like something you mentioned in #36379 (comment) and #36379 (comment).
packages/edit-site/src/components/add-new-template/new-template-part.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/add-new-template/new-template.js
Outdated
Show resolved
Hide resolved
I noticed this too. Appreciate we probably can't do this here, but it would be great to immediately redirect to the template editing view with a loading state (#35503). I think it is quite important to populate the new template with some appropriate blocks.
Would it be possible to essentially duplicate the closest matching template based on the wp template hierarchy? So if I add a search template it'd duplicate Index. I feel like the "old" add template flow did this but I may be mis-remembering. |
1996ad5
to
cd5bfea
Compare
I'm not sure if it makes sense but yes I think it's possible. Probably not for this PR though. I'll add this to the overview issue. |
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 tests well for me locally. Nice job.
I agree that we need to fix how the new template appears in the list after you press add but before the page reloads.
I also agree we need to show something in the new template. Copying blocks from the parent template in the template hierarchy makes sense to me.
This is now fixed by a immediate redirection, it's still a bit slow and feels unresponsive sometimes though. |
We can use |
* Add "Add New" feature for templates list in Site Editor * Fix e2e test * Fix modal not opened when making template part * Code review * Fix unit test
I am testing the Gutenberg PR build. https://make.wordpress.org/design/2021/03/03/testing-a-gutenberg-pull-request-pr/ Templates screen I am able to add a new template for "Front Page", "Archive" and "Search" but not a custom/general template. -- Template Parts screen I am able to add a new template part: "General", "Header" and "Footer". Or perhaps there should be a custom option in Templates and Template Parts. -- The above approaches should use the same method, but we see two different methods when clicking "Add New" Should we go with one or the other approach (Modal or drop down) in the Templates and Templates Parts screen, so that we have a consistency when clicking the "Add New" button? |
@paaljoachim Since this PR is merged, I think it'd be better to surface these feedbacks in #36597 instead. |
@paaljoachim From what I understand, it's up to the theme to define the template part areas that a user can assign. In this case I think the theme only allows those three. |
Description
Based on #36379.
Implement "Add New" for templates list in Site Editor. The button opens a dropdown in the templates list page, while it opens a modal in the template parts list page. Once a template/template part is created, the page will be navigated to the created template/template part to allow further editing it. I think this makes more sense in most cases, but we can also just stay at the current page.
How has this been tested?
tt1-blocks
themeScreenshots
Templates:
Kapture.2021-11-18.at.02.29.28.mp4
Template Parts:
Kapture.2021-11-18.at.02.30.07.mp4
Types of changes
New feature
Checklist:
*.native.js
files for terms that need renaming or removal).