Nav offcanvas editor: add simple back button to inspector controls #45852
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: +165 B (0%) Total Size: 1.3 MB
ℹ️ View Unchanged
|
draganescu
reviewed
Nov 17, 2022
| return ( | ||
| <div className="block-editor-block-card"> | ||
| { isOffCanvasNavigationEditorEnabled && parentBlockClientId && ( |
There was a problem hiding this comment.
This should also work only for the navigation block
There was a problem hiding this comment.
Should it? Why not do it for all child blocks?
There was a problem hiding this comment.
I'd like to query how folks think we can do that without tightly coupling the block editor to a specific block.
Also I think it's probably great for all blocks 😄
|
I love it! I'm curious about the idea of making this accessible outside of the context of the experiment, to all blocks. I am also wondering if we want to make it a bit clearer what the button does:
cc @jameskoster @jasmussen @mtias for thoughts |
|
I like @scruffian's suggestion. 👍 I also wonder how it should work when a deeply nested link is selected. Should the back button go all the way to the navigation block or should it go to the immediate parent? I realise the first way will be harder to do, but seems more in spirit of the way navigation was intended to work. |
I'll await design feedback on your proposal @scruffian
It could do this. Bear in mind however that this will introduce Nav block specific behaviour. I can't yet see a way to avoid coupling the Nav block to the block editor package if that becomes a requirement. Any ideas for workarounds @talldan? |
|
Taking this for a spin, here's a GIF of the back button:
The good: this is a very nice interaction and useful for child to parent block navigation. Very nice. However, what's the reason to not use the navigator component that Global Styles uses? It seems built for this purpose, and by duplicating the behavior it's mostly fragmenting the experience:
I appreciate that there are technical challenges here 🙏 |
It would be very nav block specific in terms of the UI as well. It would be hard to see how jumping straight to a specific parent might apply to other blocks. So yeah, it may not be the right thing, but thought I'd ask the question. I think it is technically possible. One idea is to use a context provider within the block that provides the list view UI. The context provider would store the client id of that block (in this case the navigation block) as its value. Child blocks can check if they're in that context and then offer a back button when they are (using the client id context value to select the parent). |
@jasmussen Thanks for raising this. It's worth discussing 🙏 I was aware of this component. Indeed, to quote for the PR description: "In the future we might want to utilise the component (and friends) to achieve animation between panels". The reason I did not proceed down that route is that this is an experiment and we need to move forward quickly to get the product to a testable state. I was finding that implementing the For this reason it might well be argued that exposing this outside the experiment is not a good idea. Perhaps that's a good compromise? Update: here's my suggestion:
|
Ah yes. We could come up with some kind of generically named context and consume that within the List View components 👍 |
It'd be interesting to hear more design feedback on this one, as maybe the way it works right now is actually better? Or it could be good to start with whatever is simplest and loop back to it later after there's been testing. Also, if hardcoding something is involved I don't think that's necessarily bad—sometimes that's possible without introducing much technical debt. |
✅ Done. Created x2 Issues: |
youknowriad
reviewed
Nov 18, 2022
| const BlockInspectorSingleBlock = ( { | ||
| clientId, | ||
| blockName, | ||
| parentBlockClientId, |
There was a problem hiding this comment.
Can't we retrieve that clientId in this component instead of passing it down?
| const BlockInspectorSingleBlock = ( { clientId, blockName } ) => { | ||
| const BlockInspectorSingleBlock = ( { | ||
| clientId, | ||
| blockName, |
There was a problem hiding this comment.
Not introduced in this PR but same question for the block name
| return ( | ||
| <div className="block-editor-block-inspector"> | ||
| <BlockCard { ...blockInformation } /> | ||
| <BlockCard |
There was a problem hiding this comment.
It may simplify things if this component just received a "clientId" and handles everything using useSelect/useDispatch internally. There doesn't seem to be a valid reason for all the prop passing down (unless I'm missing something). For the record, I know this is not entirely introduced in this PR :)
There was a problem hiding this comment.
I raised #45909 to track this.
82 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?
Adds a simple "Back" icon to the inspector controls of all blocks when the Offcanvas Experiment is on which when clicked selects the parent block.
Partially addresses #45439
Why?
When you click "edit" on a menu item in the offcanvas editor it will show the inspector controls panel for that block. However it's often the case that you will then wish to return to the Navigation block you were editing.
The back button makes that easier.
How?
This is a simplified and intentionally naive implementation. The back button simply checks if there is a parent block of the currently selected block and when clicked it will select the parent block.
This gives the effect of showing the inspector controls for the parent.
In the future we might want to
<Navigator>component (and friends) to achieve animation between panelsTesting Instructions
Screenshots or screencast
Screen.Capture.on.2022-11-17.at.13-13-58.mp4