Move sectionRootClientId to block editor state #64544
Closed
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
This comment was marked as resolved.
This comment was marked as resolved.
|
Size Change: +78 B (0%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
getdave
added
[Type] Code Quality
|
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. |
getdave
commented
Aug 16, 2024
Comment on lines
+561
to
+559
| export function getSectionRootClientId( state = '' ) { | ||
| return state.sectionRootClientId; | ||
| } |
There was a problem hiding this comment.
Suggested change
| export function getSectionRootClientId( state = '' ) { | |
| return state.sectionRootClientId; | |
| } | |
| export function getSectionRootClientId( state = '' ) { | |
| let sectionRootClientId = state.sectionRootClientId; | |
| // Default to the main block if no section root is set. | |
| if ( ! sectionRootClientId ) { | |
| const blocks = getBlocksByName( state, 'core/group' ); | |
| sectionRootClientId = | |
| blocks.find( | |
| ( clientId ) => | |
| getBlockAttributes( state, clientId )?.tagName === 'main' | |
| ) ?? ''; | |
| } | |
| return sectionRootClientId; | |
| } |
I wonder if we can default the the algorithm which prefers the <main> tag?
Then if an individual editor needs to customise the behaviour it can dispatch the setSectionRootClientId() action.
Bear in the mind we'd need to account for this part of the existing algorithm in the editor package:
if ( mode === 'template-locked' ) {
_sectionRootClientId =
getBlocksByName( 'core/post-content' )?.[ 0 ] ?? '';
}
|
Thanks for working on this @getdave!
Good question. Maybe in ExperimentalEditorProvider, as that seems close to where it happened before (the editor settings pass through there), and there are some similar effects that dispatch the block rendering mode and other things. Another option is setting it right before zoom out mode is initiated, but I see that currently happens in the block editor package. There's a question of whether initiating zoom out mode like that should be a built-in part of the block editor package, or configurable on a per editor basis (e.g. it could be set by a callback like I think generally though I'd ask @youknowriad this question! |
getdave
force-pushed
the
update/move-section-root-id-to-selector
branch
from
56292e8
to
2052259
Compare
getdave
commented
Aug 30, 2024
Comment on lines
+227
to
+251
| const { computedSectionRootClientId } = useSelect( | ||
| ( select ) => { | ||
| const { getBlockAttributes, getBlocksByName } = | ||
| select( blockEditorStore ); | ||
|
|
||
| let _sectionRootClientId; | ||
|
|
||
| if ( mode === 'template-locked' ) { | ||
| _sectionRootClientId = | ||
| getBlocksByName( 'core/post-content' )?.[ 0 ] ?? ''; | ||
| } else { | ||
| _sectionRootClientId = | ||
| getBlocksByName( 'core/group' ).find( | ||
| ( clientId ) => | ||
| getBlockAttributes( clientId )?.tagName === | ||
| 'main' | ||
| ) ?? ''; | ||
| } | ||
|
|
||
| return { | ||
| computedSectionRootClientId: _sectionRootClientId, | ||
| }; | ||
| }, | ||
| [ mode ] | ||
| ); |
There was a problem hiding this comment.
Essentially this is the algorithm that determines the section "root" based on things that are specific to WordPress. For example the core/post-content block is likely WP-specific and so we can't embed this logic in the block editor package.
We could move this into a hook to better name the concept and make the code easier to reason about.
What I'm really interested to agree on is whether this is the best location to be doing such setup logic?
|
This one is very quiet. I suggest we move forward with this implementation as it's better at keeping the APIs private. If we need to reverse out in the future it should be relatively easy. Do you concur @talldan? Let's try rebasing this branch and check whether the current test failures are legit. |
|
@scruffian @jeryj would be interested in your opinions on this one. Some background in PR description and also in #64141. |
This was referenced Sep 3, 2024
|
For now I've gone with #65000. We can circle back here if/when we need to. |
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?
Moves the source of truth for
sectionRootClientIdto the block-editor store.Related to #65001
Resolves #64141
Co-authored-by: getdave [email protected]
Co-authored-by: talldan [email protected]
Why?
Currently on
trunkthesectionRootClientIdis provided to the block editor via block editor settings. This is necessary because the algorithm which determines the block to be designated as the "section root" must consider blocks that may not be present in every editor (e.g.core/post-content). Therefore to make this setting "editor agnostic" it must be passed to the block editor.However, at this stage it's important this feature is kept private as we are not yet confident in it as a public API. Therefore efforts were made to "lock" the setting. However, this seems to be non-standard and is causing errors as described in #64141.
How?
This PR seeks to explore an alternative method of providing the "section root" by moving it to block editor state with private selectors / actions to set/set the state.
Then the consuming editor instance (in WP this is the
@wordpress/editorpackage) implements it's own algorithm to determine the "section root" and dispatches an action with theclientIdof that block element it wants to use as the root.This has two advantages:
Note: I am not fully invested in this method and would be keen to understand from others whether this method is actually preferable to passing the data via block editor settings.
Testing Instructions
The aim is to verify that when in Zoom Out mode the behaviour is the same as that currently on
trunk.<main>tag.Testing Instructions for Keyboard
Screenshots or screencast