Update offcanvas back button to select parent Nav block and limited to Nav block only #46037
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
|
|
getdave
added
[Type] Code Quality
getdave
requested review from
scruffian,
MaggieCabrera,
ajlende and
draganescu
getdave
commented
Nov 24, 2022
Comment on lines
19
to
22
| title, | ||
| icon, | ||
| description, | ||
| blockType, |
There was a problem hiding this comment.
I did consider ditching all these props but they are part of the existing public API and so it didn't seem worthwhile to do this in this PR.
getdave
commented
Nov 24, 2022
| return { | ||
| parentNavBlockClientId: getBlockParentsByBlockName( | ||
| _selectedBlockClientId, | ||
| 'core/navigation', |
There was a problem hiding this comment.
FYI this does couple the block editor to the Nav block (if only for this experiment).
@youknowriad I'm wondering whether we could create a generic React Context (BlockLibraryContext?) and use it to wrap the inspector controls within the Nav block. Then we could use the context inside of block editor if it's available to get access to this kind of information without having to hard code it.
There was a problem hiding this comment.
yeah, that doesn't sound great. What are we trying to do here?
There was a problem hiding this comment.
So if I'm understanding properly:
- We're adding a button to select the parent navigation block no matter its place in the parents hierarchy right?
Some questions:
- Do we want this to be always the case, or it's a temporary thing?
- Do we want to support the back button for all blocks later down the road?
- Why not just get the direct parent?
There was a problem hiding this comment.
We're adding a button to select the parent navigation block no matter its place in the parents hierarchy right?
Yes. This relates to the offcanvas navigation editor experiment. To quote from the PR description:
Because when you edit a nested nav item in the offcanvas and then click back you expect to return to the Nav block and not the immediate parent.
Do we want this to be always the case, or it's a temporary thing?
Likely to be always the case. Currently limited to the experiment.
Do we want to support the back button for all blocks later down the road?
I'm not sure I can answer that question definitively. Some (@scruffian) have expressed a desire for this. Other's (@draganescu) have been less enamoured with it.
Subject to further user testing there is always the possibility it might be something we'd want to expose to other/all blocks.
Why not just get the direct parent?
See above. That's what it does already. But for the offcanvas editor experiment we want it to always go back to the main Navigation block because that is the block where the offcanvas editor is.
I should say that if this were exposed to other blocks then we would want to get the direct parent.
There was a problem hiding this comment.
I think deciding on whether we want to make this generic (for all blocks) (@mtias might have an answer) here is important. Because if we go generic, we can't target the "navigation block" here as this doesn't work, we can target the direct parent (which for me makes more sense)
There was a problem hiding this comment.
yeah, which could just be a property of the innerblocks component
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm not sure if that concept is mapping 1:1 with the introduction of list view. How is it being used right now? cc @jorgefilipecosta
There was a problem hiding this comment.
controlled inner blocks mean the "blocks" are saved to a different entity, so it's the case for: navigation, reusable blocks, template parts but it won't apply to "buttons", "social links"...
There was a problem hiding this comment.
I've opened this PR to track the need to make this universal.
|
Size Change: +10 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
draganescu
approved these changes
Nov 25, 2022
getdave
force-pushed
the
update/offcanvas-back-select-parent-nav-block
branch
from
3fb6060 to
b329646
Compare
|
When I don't have any navigation menus, the fallback shows a page list block with a back button. That back button goes to the template part: back.button.mp4 |
scruffian
requested changes
Nov 25, 2022
@scruffian Ok thanks for the review. So what I did here clearly working as it stands. Shall we revert that commit and ship a simple implementation as this is still an experiment? I'm conscious that we don't want to get bogged down. |
|
@getdave yeah good plan. Should we create a followup issue to look into a more generic solution? |
|
@scruffian My last commit has pushed a change which does not show button if controlling block is a template part or a reusable block. But yes we can still split this PR up. |
|
Prop drilling has been split out into #46052. I'll rebase this once that's done then take the next step. |
getdave
force-pushed
the
update/offcanvas-back-select-parent-nav-block
branch
from
3eaadce to
e9aae18
Compare
getdave
force-pushed
the
update/offcanvas-back-select-parent-nav-block
branch
2 times, most recently
from
f11d698 to
49c166d
Compare
|
@scruffian To unblock this, I've reverted the PR to the simple mechanic of checking for the Navigation block directly. Let's merge that behind the experiment flag and then I will push another branch with the more "universal" changes that folks are discussing. Can you re-review 🙏 |
This was referenced Nov 25, 2022
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?
In #45852 we added a simple back button to the offcanvas Nav block experiment which renders in the block inspector controls.
When clicked this selects the parent block. This PR updates this in two ways:
This PR also reduces the prop drilling that was introduced in #45852 as perresolved in #46052.Closes #46036
Why?
Because excessive prop drilling is a code smell.(see Reduce prop drilling in Block Card component #46052)How?
We use
getBlockParentsByBlockNameto select the closest Nav block. As the button will only display if a Nav block is detected it is therefore also only shown if the selector returns a blockId.Testing Instructions
Screenshots or screencast
Screen.Capture.on.2022-11-24.at.13-52-25.mp4