Share the editor settings between the post and site editors #55970
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
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
getdave,
jameskoster,
jorgefilipecosta,
ntsekouras and
aaronrobertshaw
youknowriad
commented
Nov 8, 2023
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { store as editSiteStore } from '../../store'; | ||
| import { unlock } from '../../lock-unlock'; | ||
| import inserterMediaCategories from './inserter-media-categories'; | ||
|
|
||
| const { useBlockEditorSettings } = unlock( editorPrivateApis ); |
There was a problem hiding this comment.
This is the existing hook that was already present in the "editor" package that I exposed temporarily to share the logic. (later I'll remove it once the site editor is able to use EditorProvider directly).
youknowriad
commented
Nov 8, 2023
| hasFixedToolbar, | ||
| keepCaretInsideBlock, | ||
|
|
||
| // I wonder if they should be set in the post editor too | ||
| __experimentalArchiveTitleTypeLabel: archiveLabels.archiveTypeLabel, | ||
| __experimentalArchiveTitleNameLabel: archiveLabels.archiveNameLabel, |
There was a problem hiding this comment.
These two settings are added by the site editor but I wonder if they make sense in the post editor. (Imagine you're using the post editor to edit the archive template, which is possible if you switch the wp_template show_admin flag to true.
I think also regardless of whether it's possible or not to actually access the page right now, we just have to think about this setting, why it's only defined in site editor but not post editor. cc @ntsekouras
There was a problem hiding this comment.
I haven't added these, but it seems they are used by Query Title to show more intuitive titles if the block is inside an archive template.
using the post editor to edit the archive template, which is possible if you switch the wp_template show_admin flag to true.
I wasn't aware of that, and I thought there is no way you can edit such a template in post editor. I think that's the reasoning it was only defined in site editor.
youknowriad
commented
Nov 8, 2023
| ] ); | ||
|
|
||
| return useBlockEditorSettings( defaultEditorSettings, postType, postId ); |
There was a problem hiding this comment.
Here I'm trying to mimic exactly what the EditorProvider is going to be doing when we use it directly.
There was a problem hiding this comment.
There's a lot of moving parts here. All the above already exists in useBlockEditorSettings? I didn't see it being added there, so assuming it already exists there.
There was a problem hiding this comment.
yes, I checked all the settings one by one.
|
Size Change: -2.12 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6e5af82. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6810641196
|
youknowriad
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
youknowriad
commented
Nov 8, 2023
| ); | ||
| export function getSettings( state ) { | ||
| // It is important that we don't inject anything into these settings locally. | ||
| return state.settings; |
There was a problem hiding this comment.
The reason for this is that we have an effect in place that calls setSettings based on the previous value of getSettings. If we add computed settings here, we'll be adding these computed settings to the state which is very unexpected.
There was a problem hiding this comment.
Maybe add the why to the comment?
talldan
reviewed
Nov 8, 2023
packages/edit-site/src/components/block-editor/use-site-editor-settings.js
Outdated
Show resolved
Hide resolved
|
This one should be ready to test and land. |
ellatrix
approved these changes
Nov 9, 2023
cbravobernal
pushed a commit
that referenced
this pull request
Nov 14, 2023
This was referenced Dec 14, 2023
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.
Related #52632
What?
The post editor and the site editor both pass down settings from the server to the block editor, they also augment these settings with some wordpress specific settings like: mediaUpload, fetchLinkSuggestions, mediaCategories...
Overtime, we found ourselves with a lot of duplication between these two editors and also in the site editor we had three different ways of providing these settings:
index.jsfile (like fetchLinkSuggestions)getSettingsselector of the edit-site storeuseSiteEditorSettingshook.So the goal of this PR is multiple:
EditorProviderdirectly in the site editor in order to be able to reuse components form the editor package as well)useSiteEditorSettings, the rest is just common between post and site editor.Note
Testing Instructions
Overall, this PR shouldn't have a big impact but it's important to test well. There are some very subtle but small changes that we may need to check, I'll try to add inline comments to the PR where I can.