Site editor sidebar: consolidate rename component and use TemplateActions component for patterns single view #54271
Conversation
ramonjd
added
[Type] Enhancement
ramonjd
changed the title
|
Size Change: +133 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 90ea73d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7294516510
|
ramonjd
force-pushed
the
try/edit-site-pattern-sidebar-actions
branch
from
cc9ed34
to
96ea2b6
Compare
ramonjd
force-pushed
the
try/edit-site-pattern-sidebar-actions
branch
from
b1cfcc0
to
314d5b7
Compare
ramonjd
changed the title
ramonjd
changed the title
ramonjd
force-pushed
the
try/edit-site-pattern-sidebar-actions
branch
from
3ca6492
to
6c110c7
Compare
packages/edit-site/src/components/sidebar-navigation-screen-template/index.js
Outdated
Show resolved
Hide resolved
ramonjd
removed
[Status] In Progress
ramonjd
requested review from
glendaviesnz,
kevin940726,
t-hamano and
aaronrobertshaw
There was a problem hiding this comment.
Appreciate the continued iterations here @ramonjd, especially when they clean up the code some ✨
✅ User-created patterns show the actions menu on their view page and it works as expected
✅ Theme patterns can't be edited directly, so aren't impacted
✅ Unedited template parts do not show an actions menu
✅ Customized template parts show an actions menu containing only "clear customizations"
✅ User-created template parts show menu and can be renamed or deleted successfully
✅ After deleting a pattern or template part I was returned to the expected page
Looks good. I left a couple of minor comments regarding possible tweaks but I'll leave those to your discretion.
Also, while testing this, it kind of stuck out that we don't have the "duplicate" option under the actions menu. What do you think of that as a further follow-up?
packages/edit-site/src/components/template-actions/rename-menu-item.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/template-actions/rename-menu-item.js
Outdated
Show resolved
Hide resolved
|
I wonder if we should hold off merging this until the 6.4 release is finalised, just so we don't diverge too much from the 16.7 RC branch if any last-minute bug fixes are needed? |
👍🏻 I'll lean on your experience. Happy to hold off and keep it rebased until we're ready. It's low priority I'd venture and mainly janitorial, though it does bring the rename/delete experience to the patterns single view, which is lacking at the moment. |
|
Thanks for testing this one @aaronrobertshaw
Yes! While I was tooling around with this PR I did dive into Follow ups
|
ramonjd
force-pushed
the
try/edit-site-pattern-sidebar-actions
branch
2 times, most recently
from
1dc41ad
to
275df92
Compare
…ete action to single pattern view in the site editor sidebar in view mode. See: #54173 (review) Starting to consolidate rename-menu-item.js Tightening up conditions. Removed unused rename component. Set decoded title var Ensure correct back path when deleting templates
- Simplified rename success notice message (removed title)
Ensure the ellipsis menus shows in patterns.
ramonjd
force-pushed
the
try/edit-site-pattern-sidebar-actions
branch
from
275df92
to
025d764
Compare
There was a problem hiding this comment.
From what I've tested, I think it's almost ready for shipping 👍
- Patterns Page:
- Theme patterns and Theme template parts can only be duplicated
- User patterns can be renamed, duplicated, exported, and deleted
- Custom template part can berenamed, duplicated, and deleted
- Page doesn't move anywhere after deleting pattern and template part
- Pattern Details Page Sidebar:
- Can be renamed, duplicated, exported, and deleted
- Return to the Patterns page after deleting the pattern
- Template Parts List Page:
- Theme template parts can only be duplicated (This is intentional, as stated in this comment)
- Can clear the theme's template part customization.
- Custom template part can be renamed, duplicated, and deleted
- Page doesn't move anywhere after deleting pattern
- Classic themes that support template parts do not display the action button.
- Template Part Details Page Sidebar:
- Custom template part can be renamed, duplicated, and deleted
- 🤔 Return to the Patterns page after deleting the template part. In trunk, return to the Template Parts List page.
- Classic themes that support template parts do not display the action button.
- Templates List Page:
- Template pages that use DataView have been stabilized by DataViews: Mark the new Templates pages as stable #57109, but this PR should have no impact as this page uses internal actions. As a follow-up, we may be able to standardize the actions on this page.
- Experimental Pages List Page:
- This PR should have no impact as the action uses a different component
- Others:
- The text field and button height of the Rename modal is 40px, so there are no regressions from the previous design improvements made in Apply __next40pxDefaultSize to TextControl and Button component in renaming UIs #56933.
- All rename actions render correctly when using emojis in the name
ramonjd
force-pushed
the
try/edit-site-pattern-sidebar-actions
branch
from
025d764
to
90ea73d
Compare
|
Thank you for testing so thoroughly @t-hamano 🙇🏻 |
Just to clarify, in this PR, after you delete a template part, it'll take you back to the previous screen:
|
Ah, I tried again and was unable to reproduce the issue. Maybe my testing procedure was wrong 🙈 |
Ha, I don't trust myself. But what I think you saw is right? I tested again and it seems okay 🤷🏻 I might get some folks who worked on patterns to also give it a test when they have free time. |
I tried it again and it seems to be fine. 8bfc3f4e6273f699f71f2454b2ee1e86.mp4 |
There was a problem hiding this comment.
Thanks for the continued effort here @ramonjd and the thorough testing @t-hamano 🙇
After a quick test and look over the code, I've left a few random thoughts and questions. I'll have more time next week to dig in and test further.
For the most part though this seems to be working pretty well. One thing that stuck out to me was the new inclusion of help text into the actions menu for clearing customizations.
It looks a bit cluttered to me and could have been the reason it was omitted when added for the patterns page in trunk. I appreciate that it is present in the template part list view's action menu, so perhaps we need some design feedback on this one?
| Trunk | This PR |
|---|---|
 |
 |
renames 'Template Actions' to 'Pattern actions' in anticipation of the great unification.
P.S. We might need a new moniker for uniting all the different types of patterns as the unification of the site and post editor has sort of usurped the name. See #57214 🙂
| // Only patterns and template parts can be duplicated for now. | ||
| const isDuplicable = isUserPattern || isNonUserPattern || isTemplatePart; | ||
|
|
||
| // If the pattern is not editable or duplicable, don't show the menu. |
There was a problem hiding this comment.
| // If the pattern is not editable or duplicable, don't show the menu. | |
| // If the pattern is not removable, revertable, or duplicable, don't show the menu. |
The current comment doesn't quite seem to fit given isEditable is defined below and not used in the conditional.
Alternatively, is it worth doing away with the comment entirely as that code block is pretty self-explanatory now?
There was a problem hiding this comment.
I was using a editable to cover all the change of state conditions, but you're right: it's just confusing noise since the vars are self-explanatory. I think deleting is a good option.
👍🏻
| { isUserPattern && ( | ||
| <MenuItem onClick={ () => exportAsJSON( record ) }> | ||
| { __( 'Export as JSON' ) } | ||
| </MenuItem> | ||
| ) } |
There was a problem hiding this comment.
If we're looking to increase parity, should we also be able to export/import user-created template parts?
There was a problem hiding this comment.
Sounds good to me. It's not a big change for this PR.
| ); | ||
| } | ||
|
|
||
| function DeleteMenuItem( { onRemove, title } ) { |
There was a problem hiding this comment.
Is it worth also splitting out the DeleteMenuItem and RevertMenuItem components to their own files to match DuplicateMenuItem & RenameMenuItem?
| export default function RenameMenuItem( { template, onClose } ) { | ||
| const title = decodeEntities( template.title.rendered ); | ||
| const [ editedTitle, setEditedTitle ] = useState( title ); | ||
| export default function RenameMenuItem( { postType, postId, onClose } ) { |
There was a problem hiding this comment.
While we're trying to make things more consistent, I noticed this gets passed the type and id separately, but in other places, we're passing an item object.
It might be good to tweak that so all these related components are consumed in an intuitive manner. Would it also prevent any excessive re-renders, not passing a fresh object around?
What do you think?
There was a problem hiding this comment.
This makes sense. Thanks for flagging. I'll take a look.
|
Thanks again for looking at this @aaronrobertshaw and @t-hamano I'll tweak the PR as suggested. One parallel effort that may put the brakes on this PR further is: The data view components take a different approach to rendering action menu. I'm not 100% sure yet, but they may not be compatible with existing action components such as these and the ones they replace. See: https://github.com/WordPress/gutenberg/blob/trunk/packages/edit-site/src/components/page-templates/index.js#L335 So if the objective is to, similar to the manage all template page, refactor patterns and other views to use dataviews, then maybe this PR isn't worth pursuing in its current form. |
Appreciate the effort you've put into this one. I have no objections if you want to close this PR, it can always be resurrected if needed. |
|
I'll close this PR - as implied above, it might create more refactor headaches as we migrate to data views. Also, the PR has been open for a while and it probably too large as it is. Therefore, I don't see the continued effort as providing commensurate value. I still think it's desirable to harmonize the actions between screens, but perhaps in a less ambitious way, and with consideration of the way dataviews consume actions. Thanks again for all the help along the way folks! |
What's happening here?
This PR:
2023-12-04.16.02.37.mp4
This PR builds on the work in:
TODO
Why?
#54173 added rename/delete actions to Template parts. Why should patterns miss out?
How?
<TemplateActions />(now renamed to<PatternActions />) to handle Pattern post types<RenameMenuItem />component<DuplicateMenuItem />componentNote: I haven't looked at the new dataview yet. It might require some refactoring if it exits experimental:
Testing
/wp-admin/site-editor.php?path=%2Fpatterns/wp-admin/site-editor.php?path=%2Fwp_template%2Falland/wp-admin/site-editor.php?path=%2Fwp_template_part%2Fall