Allow block-specific settings controls #22672
Closed
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
|
Size Change: +150 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
adamziel
commented
May 27, 2020
Comment on lines
+62
to
+63
| // Let's use non-existent ID in case the context is missing | ||
| const clientId = context?.clientId || 'non-such-id'; |
There was a problem hiding this comment.
Do we have any assertion mechanism available? Using an non-existent ID is an okayish workaround but if it's used in a block-specific mode outside of block context I'd really prefer to provide a clear error message in the console (if not blow up entirely).
6 tasks
|
Did you try you to render this fill conditionally when the block is selected: isSelected && (
<BlockSettingsMenuControls>
<MenuItem> My test menu item <MenuItem>
</BlockSettingsMenuControls>
)The issue is caused by the fact that you share one menu for all child blocks. |
|
@gziolo this would solve the problem in the post editor, but there's also the experimental menu management screen where navigator items have an ellipsis menu that's clickable even when the block is not selected:
In theory we could just select the block when the menu is clicked, but it would shift the focus from the navigator over to the editor area which would be a bad experience (especially for users using primarily their keyboards). |
|
With requirements evolving rapidly, this PR may not be needed in the end. If I'm wrong, let's revisit. See also: #22089 (comment) |
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 + Test plan
The problem
Let's imagine I'd like to add an additional
MenuItemto block settings menu in acore/navigation-linkblock. I add the following snippet of code to theedit.js:Nice! This should do the trick. Let's test with a navigation link selected:
So far so good, maybe we can call it a day? Let's add one more link to be extra sure:
Uh oh, not good! Let's se what happens when the parent navigation block is selected:
Wow - that's unexpected.
Proposed solution
This behavior occurs because
BlockSettingsMenuControls.Slotdoesn't have any restrictions on block id - it just accepts all the fills from the entire component tree. This plays nicely with MenuItems that are supposed to be in every BlockSettingsMenu such as "duplicate" or "convert", but doesn't support specifying additional controls by the block itself.This PR introduces a separation between the two - the
BlockSettingsMenuControlsfill now has two modes: block-specific (default) and global. UnlessforAllBlocksproperty is set to true, the fill will have an instance-specific name and will not be used by anyBlockSettingsMenurelated to another block (or multi-block selection).Note that making block-specific mode the default feels more natural, but it also breaks BC - any third party code relying on the default global behavior will no longer work. I'd really like to get a second opinion about this decision before merging.
With this PR applied, adding the snippet of code mentioned above results in a more intuitive outcome:
If this approach gets positive feedback, I'd like to add some tests before merging this PR.
Types of changes
Breaking change (fix or feature that would cause existing functionality to not work as expected)