Add: Delete template action. #31678
Add: Delete template action. #31678
Conversation
jorgefilipecosta
changed the title
jorgefilipecosta
added
the
[Feature] Template Editing Mode
|
Size Change: +319 kB (+24%) 🚨 Total Size: 1.62 MB
ℹ️ View Unchanged
|
| ); | ||
| updateEditorSettings( { | ||
| ...editorSettings, | ||
| availableTemplates: newAvailableTemplates, |
There was a problem hiding this comment.
When creating new templates (new link there), we don't update this setting at the moment, it seems we should have that covered similarly. I think ideally, this should be a REST API request but right now patching the setting might be ok.
Also, I noticed that editorSettitngs is only used in the event handler which means we don't need to return it in useSelect, we can just use the selector with the new syntax
const { getSettings ) = useSelect( 'core/block-editor' );
There was a problem hiding this comment.
his should be a REST API request but right now patching the setting might be ok.
Right now there are filters in the core that allow plugins and themes to select which templates are available to select by the user on a post type or even a specific post. We could not just show make a rest request on the templates post type. I think we would need a specific endpoint that returns templates available for a given post where the existing core filters are executed.
There was a problem hiding this comment.
When I create a new template with the new link, save and return to the block editor,
a different template than the one I created is selected in the dropdown,
and the delete link is available for that template.
This scenario needs to be prevented because it looks like I am about to
delete the 404 template, which is an HTML file in the theme.
This is extra confusing because when I click the delete link,
the confirmation dialog does not include the name of the template that am I deleting.
-I think it would be good to include the template name. It will help against missclicks.
-I would also want to know what happens when I delete a template and it is currently in use,
but this information can be added later.
+1, this issue is becoming quite troublesome. I'd love to hear more thoughts on this from others, but I don't think the delete action should be so prominent. Deleting a template is a significant destructive action, performing it from the sidebar of the post editor feels inappropriate to me. I think I'd prefer to see something like the designs shared in #31076 (comment) |
|
What about adjusting the position of the various options....?
I am not sure if I like it or not.... A variation.
The Delete option is very prominent. Finding a way to have it visible but not have it so prominent could perhaps help. |
|
Another option to give it some more hierarchy and drag attention away from the Delete link. |
|
Ah, I wasn't referring to the visual prominence. I was questioning whether this action should be possible in the post editor at all. Personally I don't think you should be able to delete a template in the post editor, in the same way that I don't believe you should be able to delete a post in the template editor. The consequences can be severe, and enabling folks to take this action while editing a single post may mislead them in to believe that only that post will be affected. Perhaps this is something we can finesse in #31591 |
|
Good point, I guess the difficulty right now is that we'll be shipping the template mode in 5.8 without the templates UI or the site editor which means we do need a temporary solution for this in the post editor. |
|
Brainstorming. 1 - Adding the above mockup delete text link. Clicking delete will bring a popup warning saying that all pages that use the Page template will be affected. We might need a warning popup also listing pages that use the specific page template. We should look at how the Page template will likely progress and go for the most likely easy to tackle method that can be included into WP 5.8. |
|
The template editor will be in 5.8, right? So we could try something like this:
Or:
This also solves another problem: Only block templates can be deleted in the UI. So it would be peculiar to only see the delete action when certain templates are chosen in the dropdown. |
|
I like the example where the delete button is placed in the template sidebar of the template editing mode. I like it even more if this can be kept long term, maybe even in the site editor. I agree that if it is only placed centered in the top bar below the template name, there can be problems with discoverability since this placement is new. |
Hi @carolinan, that for sharing this issue. I think this issue happens because what @youknowriad said "When creating new templates (new link there), we don't update this setting at the moment". I guess we should probably fix it in a side PR. |
jorgefilipecosta
force-pushed
the
add/delete-template-action
branch
from
4183c4b
to
afa93c1
Compare
|
I updated this PR it now implements the first UI proposed by @jameskoster in #31678 (comment). I opted for the first proposal because there are plans to have the delete button at the top in the long term. From here we can expand the dropdown to include other information. |
|
Nice, this seems like a promising start to me :)
Aside from some visual styling adjustments that we can fine tune later, I have a couple of comments in relation to general functionality:
|
|
The delete link is still present in the block editor in the template section. |
|
Tested with NVDA screen reader and Chrome on Windows 10. The name of the current post or page is not read when navigating: When the button is activated, there is no context: Title of what? |
|
One more test: When I activate the button that is below the name of current page or post, |
packages/edit-post/src/components/header/template-title/delete-template.js
Show resolved
Hide resolved
packages/edit-post/src/components/header/template-title/delete-template.js
Show resolved
Hide resolved
packages/edit-post/src/components/header/template-title/delete-template.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/header/template-title/delete-template.js
Outdated
Show resolved
Hide resolved
| { __( 'About' ) } | ||
| </div> | ||
| <Button isSmall isTertiary onClick={ onToggle }> | ||
| { templateTitle } |
There was a problem hiding this comment.
What is a suitable context for this button?
What do we call the modal popover, "Template options"?
There was a problem hiding this comment.
Yes the aria-label sounds good to me 👍
packages/edit-post/src/components/header/template-title/index.js
Outdated
Show resolved
Hide resolved
| disabled | ||
| > | ||
| <MenuItem>{ templateTitle }</MenuItem> | ||
| </MenuGroup> |
There was a problem hiding this comment.
This menu is clickable and focusable but doesn't actually do anything.
I suggest not adding this as a menu until there is an action.
There was a problem hiding this comment.
Agree, the user already clicked on a button with the template name, so making this focusable doesn't add anything.
There was a problem hiding this comment.
As mentioned here, this should be an input, not a menu item.
packages/edit-post/src/components/header/template-title/delete-template.js
Outdated
Show resolved
Hide resolved
| </> | ||
| ) } | ||
| renderContent={ () => ( | ||
| <NavigableMenu> |
There was a problem hiding this comment.
If we want to achieve something like the example:
I think it's probably better to avoid using a NavigableMenu/MenuGroup/MenuItem, which should generally only be used for vertical list menus. Those components are not needed in a plain dropdown and will make it difficult to achieve the desired result. I think the screenshot above is closer to something like the Link UI in terms of the way it's presented. Arrow key navigation isn't needed for that kind of component, tabs are fine.
An example is here where the Delete button takes up the full width because it's a menu item (and some of the other styles aren't quite right):
There was a problem hiding this comment.
Although I see in the issue that this is the latest quick win design:
I still think it'd be better not to use NavigableMenu because there's only one button that can be tabbed to.
There was a problem hiding this comment.
A close button needs to be added because it is now a keyboard trap.
There was a problem hiding this comment.
Oh, there are some other designs proposed in this PR as well, not sure which is the latest 😄
A close button needs to be added because it is now a keyboard trap.
Escape or clicking outside should be fine, most popovers in the editor do this.
|
Testing. WordPress 5.7.2. Twenty Twenty One. This is what I see. Delete-template-title-drop-down.mp4It would be good to have consistency with the Site Editor template drop down: Site-Editor-Index-template-drop-down.mp4
|
|
I agree with Paal on the tooltip, but I don't know that we need the arrow 🤔 I suppose it's a question of what we want to be consistent with. The Site Editor has the arrow, but this isn't actually live yet and therefore not a familiar pattern to the average user. The post editor has the preview menu:
Which is just a button and a popover. This is pretty much the same use case as we're building here, so it may make more sense to be consistent with that for now? |
|
As we need to have the delete template feature in 5.8. I would say go with what feels natural...:) |
…-template.js Co-authored-by: Carolina Nymark <hi@themesbycarolina.com>
…-template.js Co-authored-by: Carolina Nymark <hi@themesbycarolina.com>
…-template.js Co-authored-by: Carolina Nymark <hi@themesbycarolina.com>
…-template.js Co-authored-by: Carolina Nymark <hi@themesbycarolina.com>
Co-authored-by: Carolina Nymark <hi@themesbycarolina.com>
…-template.js Co-authored-by: Carolina Nymark <hi@themesbycarolina.com>
|
Hi @carolinan thank you a lot for all the code suggestions you proposed for this PR 👍 They were applied. This PR was updated it now implements an input field that also allows the change of the template title as suggested. The design is not there yet and we may need to do some follow-ups but I feel this PR implements the required functionality and should be ready of 10.7. We can after iterate and cherry-pick the additional PR's. |
Co-authored-by: Carolina Nymark <hi@themesbycarolina.com>
Part of #31076.
This PR implements a delete action for page templates.
For now, the UI is a simple action link like the other actions (edit and new), in the future we may iterate on the design.
How has this been tested?
I verified I could delete user-created templates.