Consolidate template actions components #59586
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
ntsekouras
added
the
[Type] Code Quality
ntsekouras
requested review from
Mamaduka,
youknowriad,
jameskoster,
SaxonF,
jorgefilipecosta and
t-hamano
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -1.62 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
It seems there was a small difference between the two components. In sidebar navigation when we |
|
Good question. As we look to consolidate the details panel and the Inspector it would be good to align the interaction at some point. I don't necessarily think it has to happen right now but if it's simple enough to implement I'd lean towards treating 'Clear customizations' like a delete action. IE it is instantaneous, but shows a confirm dialog. I think the confirm dialog is particularly important in the editing context, given that most changes are not published until you save. |
This comment was marked as off-topic.
This comment was marked as off-topic.
That seems okay to me, as long as we display a confirm dialog which alerts the user to the gravity of the action, and offers a way to cancel. FWIW I'm not seeing a confirm dialog on this PR, but I am seeing the immediate saving, and undo doesn't seem to work – it just empties the whole template: clear.mp4I think the thing to avoid is different behaviors in different contexts, given we want to consolidate the Inspector + details panel, and potentially provide access to it from data views for quick editing. |
Oh.. I'm sorry about that. I was thinking the |
|
I updated the code to use a confirm dialog when clearing the customizations and I updated the e2e tests. As a follow up we should see to be more consistent with the |
|
I see the confirm dialog now 👍 There's still an issue where the template is emptied on undo though. To replicate:
I think we could probably use 'Reset' instead of 'Clear customizations' for templates, and move the "Use template as supplied by the theme" messaging to the confirm dialog. Quick mockup:
|
@jameskoster is the same issue on trunk too? Because I couldn't replicate and we have e2e tests for that too.. |
|
Yes I see it on trunk too 🤔 trunk.mp4 |
Hm.. okay. Probably it's better to address this separately then.
We can do that in a follow up, no? |
t-hamano
reviewed
Mar 6, 2024
packages/edit-site/src/components/sidebar-edit-mode/template-panel/index.js
Outdated
Show resolved
Hide resolved
…anel/index.js Co-authored-by: Aki Hamano <[email protected]>
Definitely 👍 |
jorgefilipecosta
approved these changes
Mar 6, 2024
| </DropdownMenu> | ||
| <> | ||
| <MenuItem | ||
| info={ __( 'Use the template as supplied by the theme.' ) } |
There was a problem hiding this comment.
This text is also showing for template parts. Should we have a conditional message and show "Use the template-part as supplied by the theme."?
There was a problem hiding this comment.
Let's do that in a follow up that will update the confirmation modal for this.
|
@jameskoster in your suggested design for updating the confirm dialog, it's quite different in terms of design with all the other usages we have in our codebase. Although I find the new design better personally, should we add it like this right now for just this use case? Do we plan on updating the other similar dialog components and/or even consider updating the |
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.
What?
This PR refactors the code to use the same
TemplateActionscomponent, that is used in templates and template partsmoremenu(ellipsis). Currently there is a mismatch in provided actions between the sidebar navigation and the template/template part cart menu.Notes
In trunk
TemplateActionsare also used for patterns, which is not ideal since it works for template parts but not for the rest of patterns. We need to provide the available actions there too for patterns.Testing Instructions
Screenshots or screencast
Screen.Recording.2024-03-05.at.12.03.08.PM.mov