Site Editor: Template list add rename action #36879
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
requested review from
talldan,
kevin940726 and
jameskoster
and removed request for
talldan
Mamaduka
added
[Feature] Site Editor
|
Size Change: +379 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
talldan
reviewed
Nov 26, 2021
packages/edit-site/src/components/list/actions/rename-menu-item.js
Outdated
Show resolved
Hide resolved
|
Thanks for testing, @talldan. I also noticed that focused state and delete button styles don't work well side by side. |
kevin940726
reviewed
Nov 26, 2021
| <> | ||
| <MenuItem | ||
| onClick={ () => { | ||
| setIsModalOpen( true ); |
There was a problem hiding this comment.
Why not calling onClose here once the modal is opened?
There was a problem hiding this comment.
Calling onClose here will unmount dropdown, so modal is never rendered.
There was a problem hiding this comment.
I see. Then why aren't we rendering the modal outside of the dropdown?
There was a problem hiding this comment.
No strong preference for this. I just thought having a separate component would be a little cleaner. Happy to move logic in the main file. Probably going to save a few lines of code as well :)
There was a problem hiding this comment.
BTW, I don't think we should be closing the modal for accessibility reasons. Closing the modal should move focus back to the Rename button again.
There was a problem hiding this comment.
I also like that behavior but wasn't sure if it was accessibility improvement.
| onRequestClose={ () => { | ||
| setIsModalOpen( false ); | ||
| } } | ||
| overlayClassName="edit-site-template__modal" |
There was a problem hiding this comment.
Nit: Should probably rename this to something like edit-site-list__rename-modal for disambiguation.
| <TextControl | ||
| label={ __( 'Name' ) } | ||
| value={ title } | ||
| onChange={ setTitle } |
There was a problem hiding this comment.
This should be required to avoid renaming it to an empty title.
| title={ __( 'Rename template' ) } | ||
| closeLabel={ __( 'Close' ) } | ||
| onRequestClose={ () => { | ||
| setIsModalOpen( false ); |
There was a problem hiding this comment.
This seems a little weird to me, as I'd imagine closing the modal would also remove previously edited title. I believe that's the side-effect of using useEntityProp on every change? We could create a new state and reset it every time the modal is opened though.
| onClose(); | ||
|
|
||
| // Persist edited entity. | ||
| await saveEditedEntityRecord( 'postType', template.type, template.id ); |
There was a problem hiding this comment.
We should probably also add a isLoading state and set isBusy to the submit button. And once the change is saved, we can then close the modal and potentially display success or error notices as in #36808.
There was a problem hiding this comment.
I don't think there's a need for the busy state. I'm closing the modal before the HTTP request is made to persist changes. So there shouldn't be a visible busy state.
I've updated the code to display notices.
P.S. There should be an easier way to use try/catch with actions.
Mamaduka
force-pushed
the
add/rename-template-modal
branch
from
8d6c48b
to
802a14b
Compare
|
Nice! I noticed one peculiar interaction: If you open the rename modal, the input value = the current template name which makes sense. If you then cancel and immediately re-open, the input value is empty. Just a minor detail but would be nice to fix if possible. I also noticed that I am able to rename custom primary templates (front-page, archive, search etc). I don't think we should allow this as it creates potential for folks to confuse themselves. Only from-scratch Agree that we should remove the border on the delete button. We can do that separately though. |
|
Thanks for catching those issues, @jameskoster. They should be fixed now.
Sounds good. |
|
The input issue is fixed 👍 I'm still able to rename my search / front-page templates though. |
|
Ahh the 3 dot menu does not show on core templates only on user or theme created templates. I made a template Archive and noticed the 3 dots show up. This is what I see:
Good to hear that the red border will be removed! -- EDIT:
Which seems like a fitting text. |
|
It looks like I forgot to add |
Mamaduka
force-pushed
the
add/rename-template-modal
branch
from
cd92f84
to
1d68794
Compare
|
@jameskoster, the custom template issue should be fixed now. |
jameskoster
approved these changes
Nov 26, 2021
Mamaduka
force-pushed
the
add/rename-template-modal
branch
from
1d68794
to
63719ab
Compare
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Nov 29, 2021
* Site Editor: Template list add rename action * Update modal class * Address feedback * Only allow renaming of user created templates * Fix empty title issue * Fix condition * Use is_custom
20 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Part of #36773.
Adds new "Rename" action to the template list that will display modal. The modal design matches "Create new template" and "Add to Reusable blocks" modal designs.
Notes
How has this been tested?
Screenshots
Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).