Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sync settings from editor store to block-editor store #47287

Merged
merged 4 commits into from
Jan 27, 2023
Merged

Sync settings from editor store to block-editor store #47287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c2c22b7
useBlockEditorSettings: stop filtering editor settings when syncing
jsnajdr Jan 25, 2023
8673a94
Sync settings from editor store to block-editor store
jsnajdr Jan 19, 2023
047c1b7
Reset supportsLayout default
jsnajdr Jan 20, 2023
8943ba3
Send updates to editor settings and let them sync to block editor
jsnajdr Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions packages/editor/src/components/provider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,22 +39,29 @@ export const ExperimentalEditorProvider = withRegistryProvider(
}
return { postId: post.id, postType: post.type };
}, [ post.id, post.type ] );
const { selection, isReady } = useSelect( ( select ) => {
const { getEditorSelection, __unstableIsEditorReady } =
select( editorStore );
return {
isReady: __unstableIsEditorReady(),
selection: getEditorSelection(),
};
}, [] );
const { editorSettings, selection, isReady } = useSelect(
( select ) => {
const {
getEditorSettings,
getEditorSelection,
__unstableIsEditorReady,
} = select( editorStore );
return {
editorSettings: getEditorSettings(),
isReady: __unstableIsEditorReady(),
selection: getEditorSelection(),
};
},
[]
);
const { id, type } = __unstableTemplate ?? post;
const [ blocks, onInput, onChange ] = useEntityBlockEditor(
'postType',
type,
{ id }
);
const editorSettings = useBlockEditorSettings(
settings,
const blockEditorSettings = useBlockEditorSettings(
editorSettings,
!! __unstableTemplate
);
const {
Expand Down Expand Up @@ -119,7 +126,7 @@ export const ExperimentalEditorProvider = withRegistryProvider(
onChange={ onChange }
onInput={ onInput }
selection={ selection }
settings={ editorSettings }
settings={ blockEditorSettings }
useSubRegistry={ false }
>
{ children }
Expand Down
36 changes: 21 additions & 15 deletions packages/editor/src/components/provider/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,15 @@ class NativeEditorProvider extends Component {
}

componentDidMount() {
const { capabilities, locale, hostAppNamespace, updateSettings } =
this.props;
const {
capabilities,
locale,
hostAppNamespace,
updateEditorSettings,
updateBlockEditorSettings,
} = this.props;

updateSettings( {
updateEditorSettings( {
...capabilities,
...this.getThemeColors( this.props ),
locale,
Expand Down Expand Up @@ -152,7 +157,9 @@ class NativeEditorProvider extends Component {
window.wp.galleryBlockV2Enabled =
galleryWithImageBlocks;
}
updateSettings( this.getThemeColors( editorSettings ) );
updateEditorSettings(
this.getThemeColors( editorSettings )
);
}
);

Expand All @@ -177,7 +184,7 @@ class NativeEditorProvider extends Component {
const impressions = { ...NEW_BLOCK_TYPES, ...storedImpressions };

// Persist impressions to JavaScript store.
updateSettings( { impressions } );
updateBlockEditorSettings( { impressions } );

// Persist impressions to native store if they do not include latest
// `NEW_BLOCK_TYPES` configuration.
Expand Down Expand Up @@ -318,7 +325,7 @@ class NativeEditorProvider extends Component {
}

updateCapabilitiesAction( capabilities ) {
this.props.updateSettings( capabilities );
this.props.updateEditorSettings( capabilities );
}

render() {
Expand Down Expand Up @@ -352,17 +359,14 @@ const ComposedNativeProvider = compose( [
getEditorBlocks,
getEditedPostAttribute,
getEditedPostContent,
getEditorSettings,
} = select( editorStore );
const { getEditorMode } = select( editPostStore );

const {
getBlockIndex,
getSelectedBlockClientId,
getGlobalBlockCount,
getSettings: getBlockEditorSettings,
} = select( blockEditorStore );
const { getBlockIndex, getSelectedBlockClientId, getGlobalBlockCount } =
select( blockEditorStore );

const settings = getBlockEditorSettings();
const settings = getEditorSettings();
const defaultEditorColors = settings?.colors ?? [];
const defaultEditorGradients = settings?.gradients ?? [];

Expand All @@ -381,7 +385,8 @@ const ComposedNativeProvider = compose( [
};
} ),
withDispatch( ( dispatch ) => {
const { editPost, resetEditorBlocks } = dispatch( editorStore );
const { editPost, resetEditorBlocks, updateEditorSettings } =
dispatch( editorStore );
const {
updateSettings,
clearSelectedBlock,
Expand All @@ -393,7 +398,8 @@ const ComposedNativeProvider = compose( [
const { createSuccessNotice } = dispatch( noticesStore );

return {
updateSettings,
updateBlockEditorSettings: updateSettings,
updateEditorSettings,
addEntities,
clearSelectedBlock,
insertBlock,
Expand Down
52 changes: 1 addition & 51 deletions packages/editor/src/components/provider/use-block-editor-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,57 +131,7 @@ function useBlockEditorSettings( settings, hasTemplate ) {

return useMemo(
() => ( {
...Object.fromEntries(
Object.entries( settings ).filter( ( [ key ] ) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this has been removed, where's the filtering happening now? (Sorry I missed the ping earlier)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[
'__experimentalBlockDirectory',
'__experimentalDiscussionSettings',
'__experimentalFeatures',
'__experimentalPreferredStyleVariations',
'__experimentalSetIsInserterOpened',
'__unstableGalleryWithImageBlocks',
'alignWide',
'allowedBlockTypes',
'blockInspectorTabs',
'allowedMimeTypes',
'bodyPlaceholder',
'canLockBlocks',
'codeEditingEnabled',
'colors',
'disableCustomColors',
'disableCustomFontSizes',
'disableCustomSpacingSizes',
'disableCustomGradients',
'disableLayoutStyles',
'enableCustomLineHeight',
'enableCustomSpacing',
'enableCustomUnits',
'focusMode',
'fontSizes',
'gradients',
'generateAnchors',
'hasFixedToolbar',
'isDistractionFree',
'hasInlineToolbar',
'imageDefaultSize',
'imageDimensions',
'imageEditing',
'imageSizes',
'isRTL',
'keepCaretInsideBlock',
'maxWidth',
'onUpdateDefaultBlockStyles',
'enableOpenverseMediaCategory',
'styles',
'template',
'templateLock',
'titlePlaceholder',
'supportsLayout',
'widgetTypesToHideFromLegacyWidgetBlock',
'__unstableResolvedAssets',
].includes( key )
)
),
...settings,
mediaUpload: hasUploadPermissions ? mediaUpload : undefined,
__experimentalReusableBlocks: reusableBlocks,
__experimentalBlockPatterns: blockPatterns,
Expand Down
1 change: 0 additions & 1 deletion packages/editor/src/store/defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,4 @@ export const EDITOR_SETTINGS_DEFAULTS = {
richEditingEnabled: true,
codeEditingEnabled: true,
enableCustomFields: undefined,
supportsLayout: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reasons for the changes to the "default" values? What impact this could have?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
6 changes: 3 additions & 3 deletions test/e2e/specs/editor/various/font-size-picker.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,11 @@ test.describe( 'Font Size Picker', () => {
return doSet( obj, 0 );
}

window.wp.data.dispatch( 'core/block-editor' ).updateSettings(
window.wp.data.dispatch( 'core/editor' ).updateEditorSettings(
setDeep(
window.wp.data
.select( 'core/block-editor' )
.getSettings(),
.select( 'core/editor' )
.getEditorSettings(),
[
'__experimentalFeatures',
'typography',
Expand Down