WIP: Try surfacing content inspector controls for contentOnly inner blocks
#60412
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] Experimental
talldan
changed the title
contentOnly inner blocks
|
Size Change: +286 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Thank you for exploring this :)
I agree that this seems the most viable path to accomplish this today. But I worry if I take a step back and think though how I would want this to work if we had started with this from the beginning and I don't think adding a special group for this is where I would end up. Fundamentally this will mean that we have to duplicate controls for the regular mode and the content only mode. If backwards compatibility were no issue here I would have rather suggested we do it similarly how the toolbar for content only works today where you can get the block editing mode and then decide which controls you want to surface. But if we were to go that route that would mean that every single block ever written would need to add the check to their controls to hide any non-content relevant settings... I thought @gziolo was also exploring some ways to show/hide certain settings for the block bindings API. I wonder whether there are already any outcomes he can share on the topic :) |
|
Flaky tests detected in e08be19. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8536264007
|
I've been thinking about that too. Another drawback is that if a block wants to do something a little more custom for content only mode, like surfacing the controls in a different way or order that becomes difficult to achieve. With the conditional rendering route it can also lead to some convoluted code to avoid empty Pros and cons. I'll definitely try exploring some alternatives. |
Actually there's a good example of that already in this PR, where I'm using a single ToolsPanel to group the content controls, while normally for an image block 'title' renders in the advanced section. |
|
Pushed a commit that only drills down into blocks that have controls (e.g. image): Kapture.2024-04-05.at.17.00.28.mp4A tricky thing is figuring how to display the a chevron The image is unselected at this point so it's inspector controls also aren't rendered, and so we can't tell whether it has any or not. Schroedinger's InspectorControls. 🤔 |
22 tasks
|
Also just want to note the accessibility implications here. If you are dynamically updating the inspector, please watch for possible scenarios where we might have focus loss. Other than that, not super familiar with the site editor work these days so I'll step out. Thanks. 👍 |
Thanks @alexstine. It's a good flag, as it might be tricky to get focus management right on this PR (or at least matching user expectations). When it's a little bit more developed I'll make sure to get some feedback. |
kevin940726
reviewed
Apr 11, 2024
| /> | ||
| ) } | ||
| { !! contentOnlyFills?.length && | ||
| parentClientId && ( // This is only used by the Navigation block for now. It's not ideal having Navigation block specific code here. |
There was a problem hiding this comment.
I think we can also update the comment here.
| label={ sprintf( | ||
| // translators: %s: the block title of the parent. | ||
| __( 'Go to parent: %s' ), | ||
| parentDisplayInfo?.title |
There was a problem hiding this comment.
What should we display if the display info is still loading or it doesn't have a title here?
| { !! contentOnlyFills?.length && | ||
| parentClientId && ( // This is only used by the Navigation block for now. It's not ideal having Navigation block specific code here. | ||
| <Button | ||
| onClick={ () => selectBlock( parentClientId ) } |
There was a problem hiding this comment.
I wonder if the focus should return to the block itself or the parent <NavigationItem> instead.
| // child blocks have those controls. | ||
| const { addContentOnlyControlsBlock, removeContentOnlyControlsBlock } = | ||
| unlock( useDispatch( blockEditorStore ) ); | ||
| useEffect( () => { |
There was a problem hiding this comment.
This feels a bit weird to me (two-way syncing). Could we instead calculate the contentOnly blocks using a memoed selector? I think this would mean keeping a static list of supported blocks though 🤔.
talldan
force-pushed
the
try/content-only-drilldown
branch
from
b048414
to
904f02a
Compare
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?
See #60021, #57911
This is an exploratory PR that modifies
contentOnlymode to show some inspector controls for selected child blocks, starting with the image block. It's a potential first iteration to solving #60021 more thoroughly and solves some issues for the image block, but I also plan to explore a fuller implementation of that issue in parallel.The difference between this and
trunkforcontentOnlyparents:contentOnlyinspector controls are displayed.How?
<InspectorControls>contentOnlygroup is introduced for blocks to show content controls in the inspectorcontentOnly<InspectorControls>group to be shownSome potential changes that could improve this:
useBlockEditingMode)trunk. An arrow could be used to indicate the blocks that can be drilled into.trunkwith the toolbar controls)Testing Instructions
contentOnlygroup) and add an image and a paragraphScreenshots or screencast
Kapture.2024-04-03.at.17.16.04.mp4