Template Part: Swap block action places #42221
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
commented
Jul 7, 2022
Mamaduka
commented
Jul 7, 2022
| @@ -139,6 +139,7 @@ $z-layers: ( | |||
| ".reusable-blocks-menu-items__convert-modal": 1000001, | |||
| ".edit-site-create-template-part-modal": 1000001, | |||
| ".block-editor-block-lock-modal": 1000001, | |||
| ".block-editor-template-part__selection-modal": 1000001, | |||
There was a problem hiding this comment.
We need the modal above the dropdown.
Mamaduka
commented
Jul 7, 2022
Comment on lines
+106
to
+107
| // The `isSelected` check ensures the `BlockSettingsMenuControls` fill | ||
| // doesn't render multiple times. The block controls has similar internal check. |
There was a problem hiding this comment.
Mamaduka
commented
Jul 7, 2022
| @@ -85,6 +82,7 @@ export default function TemplatePartEdit( { | |||
| }, | |||
| [ templatePartId, clientId ] | |||
| ); | |||
| const blockTitle = useBlockDisplayTitle( clientId, 25 ); | |||
Mamaduka
added
[Type] Enhancement
|
Size Change: +601 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
I am a little hesitant to using a filter to move the Edit, but this is also where I got stuck on the issue, so if you think it is the best way, it works. |
carolinan
reviewed
Jul 7, 2022
| > | ||
| { sprintf( | ||
| /* translators: %s: block name */ | ||
| __( 'Replace %s' ), |
I don't see any better alternatives at the moment. The "Edit" action code should live inside the site editor package at least for now. We're already using this filter to enhance blocks in the core.
Maybe |
Yes I think that would make it clearer. |
jameskoster
approved these changes
Jul 7, 2022
|
@jameskoster, I will have a look next week. But I don't think we have a SlotFill in that component. |
|
In that case I suppose this would be an acceptable interim, but we should prioritise moving replace to the transform menu and avoid 'crossing the streams' between transforms and other actions. |
Mamaduka
force-pushed
the
try/template-part-swap-edit-replace
branch
from
9d75bb7
to
a5ced2b
Compare
andrewserong
approved these changes
Jul 14, 2022
There was a problem hiding this comment.
This is working nicely for me @Mamaduka. I'm not too familiar with this particular part of the site editor, but the code change reads well to me, and the decisions you've made make good sense 👍
The "Replace" action is now uses BlockSettingsMenuControls SlotFill, directly block block edit component
Seems like an acceptable approach to me, particularly since we expect to do that with the toolbar and inspector controls from blocks' edit components 👍
This is testing well in the site editor, and "Edit" feels like a much more common action to take on a template part than replace, so sounds like a good switch. Here's how it's looking for me:
| Before | After |
|---|---|
 |
 |
And editing and replacing template parts still works correctly as on trunk. LGTM! ✨
| 'withEditBlockControls' | ||
| ); | ||
|
|
||
| addFilter( |
There was a problem hiding this comment.
Interesting choice using the filter here. I don't mind it personally, and like you mention, there's a parallel with the move-to-widget-area. If it weren't for the useLink and useLocation imports, I'd say it might be good to see if we can somehow get it into the block itself in block-library, but maybe not worth the effort of untangling all that since the filter appears to work well 👍
There was a problem hiding this comment.
If it weren't for the useLink and useLocation imports...
This is the exact reason I decided to use the filter, plus the "Edit" action is currently Site Editor only.
There was a problem hiding this comment.
Sounds fair, particularly given that template parts is very site editor centric — so sounds like a reasonable argument for the site editor to have special handling for this block!
There was a problem hiding this comment.
Thanks for the review, @andrewserong 🙇
ntsekouras
reviewed
Jul 14, 2022
ntsekouras
reviewed
Jul 14, 2022
ntsekouras
approved these changes
Jul 14, 2022
There was a problem hiding this comment.
Thanks for your work here George! Besides the BlockTitle everything looks good to me! I'm pre-approving 😄
13 tasks
38 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.
What?
Resolves #41437.
Resolves #34737.
PR swaps places for "Edit" and "Replace" template part actions.
Why?
The "Edit" should be the primary action for the Template Part block.
How?
The Site Editor now injects the "Edit" action via a filter. We use a similar method for the Widgets screen to inject the "Move to" action. I've removed old component.
The "Replace" action is now uses
BlockSettingsMenuControlsSlotFill, directly block block edit component. I've not seen this slot used directly from a block before (at least in core), so I would love to hear your opinion on this one 🙇Testing Instructions
Screenshots or screencast
CleanShot.2022-07-07.at.13.10.49.mp4