Provide default algorithm for section root in block editor #65516
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] Enhancement
getdave
requested review from
draganescu,
youknowriad,
talldan and
MaggieCabrera
|
I'm thinking we should add some tests for this selector. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +29 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Sep 20, 2024
| // cases where the provided setting is an empty string to signify | ||
| // the "root block" of the editor. | ||
| if ( settingsRootClientId !== undefined ) { | ||
| return settingsRootClientId; |
There was a problem hiding this comment.
Do we explicitly provide "" as a settingsRootClientId in the post editor today or do we leave that to the block-editor's algorithm?
There was a problem hiding this comment.
As @wordpress/editor provides the setting, today Site and Post Editors would use the algorithm that looks for main.
However, if the rendering mode is template-locked (like when you are editing a Page in the Site Editor) then the default would be '' (empty string).
I think the question is here
There was a problem hiding this comment.
Main main question is about "performance". In other words, it seems unnecessary to run the fallback for post and page editors in "post-only" mode (not template-locked), for template-locked, I suspect that we already have something that sets the "post-content" block as section root.
There was a problem hiding this comment.
I see what you mean. If we are in Post Editor (Posts or Pages or whatever) we are in post-only mode. Therefore we're going to be looking for a main tag which is very unlikely to exist.
Yes maybe I need to do some more digging to find where these editors end up with '' as the sectionRootClientId.
getdave
commented
Sep 20, 2024
packages/editor/src/components/provider/use-block-editor-settings.js
Outdated
Show resolved
Hide resolved
draganescu
reviewed
Sep 20, 2024
It's a good question to ask.
The setting is private so you have to unlock and do the dance to be able to use it.
That's true (with caveat above). However, I felt that if a feature is part of the block editor package then it should work (at least to an extend) without relying on a setting passed in. I'm happy to be told I'm wrong, but it seems logic to have a "fallback" that "just works" rather than each implementor reinventing the wheel. |
|
@draganescu Any thoughts about my responses above? Would be helpful whilst I move forward here. Thanks 🙇 |
|
I think the best temporary solution IMO is to not show the affordance if the setting is not set as well? My reasoning is that the editor implementation not the block editor decides what sections are. |
|
Ok if that's the case we'll need to disable zoom out and edit mode entirely if that setting is not provided. |
|
Seems more fair - at least for now when this feature is so new. |
|
Ok I'll raise a separate PR. |
|
Connecting the dots with #67232 |
getdave
force-pushed
the
update/provide-default-algorithm-for-section-root
branch
from
e06a1c0 to
61f62e1
Compare
|
I've realised that since @talldan's work to move block editing modes to the reducer the This means the selector is no longer canonical for retrieving the section root. This is because - i suppose - this I considered moving the So overall I'm no longer convinced we should offer a default implementation for the section root client ID and rather simply let the Editor decide what is the root. |
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?
Closes #65400.
Adds a WordPress-agnostic default algorithm for determining "section root" in the block editor.
Continues to allow override via setting.
Why?
Currently if the setting that provides the
sectionRootClientIdis not provided then the editor experience in "Zoom Out" is quite degraded. You just end up with the root block selectable and nothing else.By providing a default algorithm that is WordPress agnostic, we allow this block editor based feature to function "standalone" whilst still allow for WordPress to provide its own algorithm as required.
How?
maininto the block editor package.undefined).Testing Instructions
On trunk
undefinedgutenberg/packages/editor/src/components/provider/use-block-editor-settings.js
Line 192 in 5e37a13
On this PR
sectionRootClientIdsetting and confirm everything still works as pertrunk.Testing Instructions for Keyboard
Screenshots or screencast