Sync settings from editor store to block-editor store #47287
Merged
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: -381 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
youknowriad
approved these changes
Jan 19, 2023
jsnajdr
force-pushed
the
update/sync-editor-settings
branch
from
5fca8e7
to
efaf822
Compare
|
Something in the mobile editor breaks with this patch, I need to have a look. |
youknowriad
reviewed
Jan 20, 2023
| @@ -27,5 +27,4 @@ export const EDITOR_SETTINGS_DEFAULTS = { | |||
| richEditingEnabled: true, | |||
| codeEditingEnabled: true, | |||
| enableCustomFields: undefined, | |||
| supportsLayout: true, | |||
There was a problem hiding this comment.
Any reasons for the changes to the "default" values? What impact this could have?
There was a problem hiding this comment.
This value is wrong for the mobile editor. It should be inherited from default block editor settings (the ...SETTINGS_DEFAULTS spread a few lines above) where it's supportsValue: false for the mobile editor.
Until now it didn't matter because nobody read the supportsValue from the editor settings. But now editor settings are synchronized to block editor settings, and the bad value is propagated there. And it affects whether the "wide" and "full" block alignment options are displayed.
|
Flaky tests detected in fa6a431238aec37c0caa9253c5754ec8fa1c27c9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4004446149
|
jsnajdr
commented
Jan 20, 2023
| text: true, | ||
| background: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I most likely won't keep this because it probably disables text color settings for non-block themes, which is not wanted. See #34633 that introduced these lines I'm deleting. I just want to know if it fixes all the mobile unit and e2e tests, then I'll try to figure out a better fix.
jsnajdr
force-pushed
the
update/sync-editor-settings
branch
3 times, most recently
from
91cb1bc
to
fa6a431
Compare
jsnajdr
force-pushed
the
update/sync-editor-settings
branch
from
fa6a431
to
0dd56db
Compare
|
Hi @youknowriad 👋 This is now ready for final review. |
jsnajdr
force-pushed
the
update/sync-editor-settings
branch
from
0dd56db
to
8943ba3
Compare
youknowriad
reviewed
Jan 27, 2023
| @@ -131,57 +131,7 @@ function useBlockEditorSettings( settings, hasTemplate ) { | |||
|
|
|||
| return useMemo( | |||
| () => ( { | |||
| ...Object.fromEntries( | |||
| Object.entries( settings ).filter( ( [ key ] ) => | |||
There was a problem hiding this comment.
I see this has been removed, where's the filtering happening now? (Sorry I missed the ping earlier)
There was a problem hiding this comment.
There's no filtering now, the entire editor settings object will be propagated to block editor settings. Technically it causes no harm, or does it? I can see one good reason for filtering, and that is a reliable and well-defined public API, but that's all.
I removed the filtering because it was standing in the way of synchronizing settings in the mobile EditorProvider. Before this PR, it called dispatch( 'core/block-editor').updateSettings directly, and circumvented the filter. But now it calls dispatch( 'core/editor' ).updateEditorSettings and relies on the settings to be synced to block editor. Here the updates go through the filter.
I found it hard to figure out the set of supported mobile block editor settings, see e.g. the capabilities confusion solved in #47417. Once all this is sorted out, I'll be happy to add the filter back.
There was a problem hiding this comment.
There's no filtering now, the entire editor settings object will be propagated to block editor settings. Technically it causes no harm, or does it?
Well now, folks will be able to access settings using `select( 'core/block-editor' ).getSettings() that were specific to the edit-post package and it creates wrong expectations. Ideally only block-editor specific settings should be passed down.
There was a problem hiding this comment.
I think the important thing here is that the filtering is going to be needed for 6.2 (so before next Gutenberg RC) if we don't backward compatibility issues.
There was a problem hiding this comment.
OK I'll add it back ASAP. Note that it's possible to circumvent this filter and set an arbitrary block-editor setting with dispatch( 'core/block-editor').updateSettings(). Mobile editor does that with capabilities and other settings: capabilities is read by the editor package, by specific blocks ("image" and "missing") and by rich-text (to implement support for mentions and xposts, which seems Jetpack or A8c-specific to me?)
There was a problem hiding this comment.
yeah, I mean folks might ask us why enableCustomFields for instance is not available anymore in the "site editor" or something while it's an edit-post specific flag...
Granted that it's not perfect, I guess I'm just leaning towards a defensive mode here in terms of what's exposed and where.
juanmaguitar
added
the
[Type] Code Quality
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.
This PR changes how editor settings are synced and propagated from the top-level
Editorcomponent's props, into thecore/editorstore, and to thecore/block-editorstore. It's to fix various race conditions and e2e failures described in #46467 (comment)The changes are:
<Editor settings/>prop toeditorstore andblock-store, we only sync it toeditorstore, and then set up a sync fromeditortoblock-editor. It's now a chain, not two parallel syncs. That means that when updating theeditorsettings, they will be reliably synced to theblock-editorsettings.supportsLayoutdefault foreditorsettings, as discussed in 0451839#r1082467189NativeEditorProvidercomponent also needed an update. Previously it updated theblock-editorsettings, which caused race conditions with theeditor->block-editorsync. Especially for the styles in__experimentalFeatureswhere various updates were overwriting each other. Now it updates theeditorsettings instead, and sync toblock-editorwill happen automatically. The onlyblock-editorupdate is forimpressions, which are never passed from outside.useBlockEditorSettings, I removed "filtering" the list of settings fields to be synchronized fromeditortoblock-editor. Now we sync the entire settings bag toblock-editor. The reason is that it's very difficult to figure out the "right" list ofblock-settings(see thecapabilitiesconfusion in Mobile: cleanup usage of settings.capabilities #47417), the list needs to updated often, and it does no harm to just pass the entire settings bag without filtering.