Add content only descriptions in dropdown menus for patterns and templates #61127
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
kevin940726
added
[Type] Enhancement
|
Size Change: +2.18 kB (+0.13%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Do we still want the "Create template part" action in the above screenshot? Also, I noticed that the "Content" block shows the "Move to" action and can be grabbed but can't actually be moved anywhere. Not sure if this is a bug or not.
|
This was referenced Apr 26, 2024
kevin940726
changed the base branch from
update/content-only-lock-icon
to
trunk
kevin940726
force-pushed
the
update/content-only-menu-desc
branch
from
092fea8 to
9447fba
Compare
|
Can work. I wonder if we can tweak the verbiage a bit. Instead of:
perhaps:
|
@jasmussen What do you think about a small tweak for this part? - "Edit the pattern to modify its placement or delete it." |
|
Even better, ship it. |
Seems like a bug.
99% of the time this has very little practical use. I reckon we could try removing it.
Is "modify its placement" a bit verbose? Could be 'This block belongs to "[pattern-name]", edit the pattern to move or delete it'. |
kevin940726
force-pushed
the
update/content-only-menu-desc
branch
from
bc47175 to
318a493
Compare
kevin940726
commented
Apr 29, 2024
| @@ -1752,14 +1752,16 @@ export function canMoveBlock( state, clientId, rootClientId = null ) { | |||
| if ( attributes === null ) { | |||
| return true; | |||
| } | |||
| if ( getBlockEditingMode( state, rootClientId ) !== 'default' ) { | |||
There was a problem hiding this comment.
Move the block editing check earlier so that blocks in the page editor won't be moved (which is expected.)
packages/editor/src/components/block-settings-menu/content-only-settings-menu.native.js
Show resolved
Hide resolved
| @@ -0,0 +1,4 @@ | |||
| .editor-content-only-settings-menu__description { | |||
| padding: $grid-unit; | |||
| min-width: 235px; | |||
There was a problem hiding this comment.
An arbitrary width to match the current menu's width with icons.
packages/edit-site/src/components/template-part-converter/index.js
Outdated
Show resolved
Hide resolved
|
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. |
kevin940726
force-pushed
the
update/content-only-menu-desc
branch
from
318a493 to
2bdd5f2
Compare
|
Flaky tests detected in 04ba4e8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8920800995
|
talldan
reviewed
Apr 30, 2024
There was a problem hiding this comment.
This looks great, it's a really nice affordance to explain the various modes to users.
I noticed three things that I think are bugs.
One I mentioned in a comment. If you have a contentOnly locked group, the wrong template message shows in a dropdown, which is incorrect. Might need more design feedback on that aspect to know what to show:
The second is that when editing a pattern, button blocks with overrides show the 'Add before'/'Add after' buttons, which isn't something that will work:
The third is that when editing a page, blocks like 'Title', 'Featured Image' and 'Content' show the Convert to Template Part option, which I imagine is not desirable and it probably also wouldn't work as those blocks are not replaceable:
packages/editor/src/components/block-settings-menu/content-only-settings-menu.js
Outdated
Show resolved
Hide resolved
Comment on lines
70
to
88
| <Text | ||
| variant="muted" | ||
| as="p" | ||
| className="editor-content-only-settings-menu__description" | ||
| > |
There was a problem hiding this comment.
This might be something to consider the accessibility of. I don't know how discoverable text would be in a menu like this. It might be fine, or it might be better to also announce the message as an aria-description or something, that's something that might need further feedback.
There was a problem hiding this comment.
Good point. This may need to be a more standard popover rather than a menu.
There was a problem hiding this comment.
@kevin940726 is it feasible to move the text outside of the role="menu" container, and give it a different role?
There was a problem hiding this comment.
I don't think it's easily achievable 🤔. Do you think it can be done in a follow-up?
I think I fixed this back in f824d5a. Can you still reproduce it? |
These two are fixed in 8b4ae5e. |
|
The wordings are not final. We can work on that or push a follow-up PR if we haven't decided yet. Both sound good to me! |
This particular example reads a little strange to me. I wonder if this would work better:
Aside from that small tweak I think this is good to try. It feels like a solid improvement on the lock icon to me. |
kevin940726
force-pushed
the
update/content-only-menu-desc
branch
from
83a76c9 to
0147d37
Compare
Updated in a4b4877. |
|
How about we simplify a bit, aligning with the Edit/Select mode popover, placing controls before the help text.
|
|
In a767bd1, I updated the text and removed the references of the block names as suggested in #61127 (comment). Template Lock: Patterns Templates |
talldan
approved these changes
May 10, 2024
| @@ -391,3 +391,22 @@ export function expandBlock( clientId ) { | |||
| clientId, | |||
| }; | |||
| } | |||
|
|
|||
| export const modifyContentLockBlock = | |||
There was a problem hiding this comment.
It'd be good to add a docblock
packages/editor/src/components/block-settings-menu/content-only-settings-menu.native.js
Show resolved
Hide resolved
|
This PR caused a regression in terms of which blocks are locked or not in template-locked mode. Before: 
After: 
|
Mamaduka
reviewed
Jun 6, 2024
| canInsertBlock: canInsertDefaultBlock || !! directInsertBlock, | ||
| canInsertBlock: | ||
| ( canInsertDefaultBlock || !! directInsertBlock ) && | ||
| rootBlockEditingMode === 'default', |
There was a problem hiding this comment.
This check turned out to be too restrictive. See #62367.
2 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?
Based on #61012. Implement the design in #60834 (comment).
Add descriptions for content-only blocks inside patterns or templates in the dropdown menu.
Why?
See #60834.
This is a UI enhancement for content-only blocks. Most of the actions in the original dropdown aren't needed for content-only blocks. We remove them and provide descriptions and links to edit the entities instead.
How?
Hide original actions and inject descriptions for content-only blocks. The wording and the design are still TBD and can be tweaked.
Testing Instructions
For patterns:
For templates:
Screenshots or screencast
Patterns:
Templates:
