Make the __experimentalCaptureToolbars option work reliably #28905
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
talldan
added
[Type] Bug
|
Size Change: +18 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
This feels more consistent when editing Navigations with submenus. One thing I did notice was that the parent button icon changes; When I'm editing a top-level link the parent button shows the Navigation block icon, but when I select a child of a top-level link the icon shows the Link block icon:
I understand why this happens, and that this PR doesn't actually change this — the same thing happens in Maybe it should always show the root container (the Navigation block) as the parent? Or, maybe this would make more sense if there was an imaginary Sub-menu block? Either way, I think this PR should merge. 🚢 |
tellthemachines
approved these changes
Feb 11, 2021
There was a problem hiding this comment.
Code looks good and it's working well ✅
Fwiw, I'm not bothered by the parent selector selecting the direct parent in a deeply nested situation, as both the visible outline around the currently selected block and the closing of submenu levels make it pretty clear what's going on.
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
Fixes #28873
The
__experimentalCaptureToolbarsmakes child block toolbars appear in the position of a parent block. However, the setting seems to work unreliably, sometimes the toolbar appears over a random block.The problem is that the feature uses a
__experimentalGetBlockListSettingsForBlocksselector, and it seems the assumption is that this selector would return blocks in the same order as theclientIdsarray passed in as the argument.That's not the case since the selector derives its return value from the
blockListSettingsstate, which is an unordered associative array style object that maps clientIds to settings. The return value of the selector is also a compact array—where a block has no settings it isn't included in the results. Both of these qualities make it impossible to reliably reference theclientIdspassed in against the results of the selector.This PR changes
__experimentalGetBlockListSettingsForBlocksso that instead of an array of block list settings, it returns an object where the keys are the requested client ids, and the values are the block setting objects. I think returning an object is the best option, since without theclientIdkeys there are no clientId values in the results at all, making the API hard to use.How has this been tested?
Updated the unit tests for the selector.
Validate that #28873 is fixed
Screenshots
Before:
After
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: