Data: Use getThemeSupports() instead of getSettings() #23566
Conversation
|
Size Change: -1 B Total Size: 1.2 MB
ℹ️ View Unchanged
|
ockham
force-pushed
the
update/use-theme-supports-instead-of-settings
branch
from
f9e0652
to
ac423c9
Compare
|
FYI @noahtallen since you're also removing initial state in #23775. |
ockham
force-pushed
the
update/use-theme-supports-instead-of-settings
branch
from
ac423c9
to
c80c1e7
Compare
ockham
force-pushed
the
update/use-theme-supports-instead-of-settings
branch
from
c80c1e7
to
13cef91
Compare
ockham
force-pushed
the
update/use-theme-supports-instead-of-settings
branch
from
fd5a451
to
64f54f5
Compare
ockham
requested review from
ellatrix,
spacedmonkey,
TimothyBJacobs and
youknowriad
as code owners
ockham
requested review from
noahtallen,
jsnajdr,
vindl,
Addison-Stavlo,
david-szabo97 and
jeyip
| @@ -89,10 +89,10 @@ export function BlockAlignmentToolbar( { | |||
|
|
|||
| export default compose( | |||
| withSelect( ( select ) => { | |||
| const { getSettings } = select( 'core/block-editor' ); | |||
| const settings = getSettings(); | |||
| const { getThemeSupports } = select( 'core' ); | |||
There was a problem hiding this comment.
The block editor package shouldn't be relying on core-data it's a network/persistence agnostic package that allows creating third-party editors. It means, this should still use getSettings.
There was a problem hiding this comment.
Just to clarify, are you saying that @wordpress/block-editor should never fetch any information from the network? In conclusion, that would mean that it'll always depend on data passed through a global variable -- i.e. it'll be always tightly coupled to server-side provided information.
But wouldn't that mean it'd be impossible to ever implement a client-side only editor?
There was a problem hiding this comment.
But wouldn't that mean it'd be impossible to ever implement a client-side only editor?
For me it's the opposite, it means that it's easier to implement client-side only editors since it doesn't need to fetch anything.
There was a problem hiding this comment.
So you would have to provide the settings object yourself via whatever method during load. That could be via PHP like in Gutenberg, but could be via your own API and init code
There was a problem hiding this comment.
The current settings are something of a loosely related set of properties from different sources:
- theme options (wide alignment, custom fonts/colors, post formats support, ...)
- image sizes (commonly affected by the theme, too)
- registered block patterns and categories
- information about the edited post's lock state
- some user options
- some site options
- customized labels for certain UI elements (
write_your_story,enter_title_here)
That alone feels suboptimal and even third-party editor authors would probably appreciate the settings being split into multiple interfaces.
For an SPA-style browser client that talks to the server only through REST API, this means waiting for some 5+ REST requests to finish before even starting to initialize the editor.
Many of them are not even used by the block-editor package, like imageSizes or imageDimensions. These could be fetched just-in-time by the blocks that need them.
Is there agreement that the settings split described above is desirable? In other words, is the objection solely about the alignWide flag (directly used by the block-editor package)?
There was a problem hiding this comment.
We should hold on doing any changes here because right now we're doing some changes to how these settings get the editor. Basically the source of truth for these will be theme.json in WordPress, they'll arrive to the block-editor in a single properly called "__experimentalFeatures" right now (subject to change). See this issue for more details #20588
Once we finish the migration there for all the properties, we'll be able to decide the best naming...
There was a problem hiding this comment.
Other things like "patterns", "lock state", "upload handler"... are different though and I believe we don't have a good answer for these at the moment.
- Patterns can become registration based like block types but I'm not sure we should commit to this for now, I'd like to give patterns more time to mature before making any formal JS APIs (they're experimental now)
- the other configs that are not theme related are more "historical". I believe most of theme still make sense as block editor settings as their meaning can be different from one editor to another but If there's concrete suggestions to change one of these we can discuss it.
That said, I believe it's a hard rule that the "block-editor" package shouldn't do any REST API call on its own.
There was a problem hiding this comment.
Fair enough. In that case, it'd be good if we could have a different mechanism to pass these options on the client side (after obtaining them through an external mechanism) -- maybe either as a prop, or maybe through a filter?
(Assuming that those settings are indeed invariant, and shouldn't be subject to change by the client, or at least not the block editor -- which makes sense for e.g. "theme supports" flags.)
|
Closing, see this discussion: #23566 (comment). Bottom line (from #23566 (comment)):
|
Description
For determining if the current theme supports
alignWide, replace calls ofselect( 'core/block-editor' ).getSettings().alignWidewithselect( 'core' ).getThemeSupports()[ 'align-wide' ], to reduce our dependency on thesettingsvariable, and move to using the REST API instead.This is a first iteration that's scoped to only tackle
alignWide. It's supposed to serve as an RFC; if people think that this is a good idea, we can make the same kind of change to a number of othersettings.How has this been tested?
Verify that unit tests are green, and do some smoke testing of the affected code.
Specifically: Make sure you're using a theme that supports wide alignment (e.g. Twenty Twenty), and insert a Cover block in the editor. Select alignment options from the block toolbar, and verify that it includes wide and full alignment. Select one of those, publish the post, and verify that the alignment is correctly applied on the frontend. Then, try the same for a theme without wide alignment support.
Furthermore, verify that there are no other instances of
alignWidethat are still coming fromselect( 'core/block-editor' ).getSettings(), except in some.native.jsfiles. (A number of remainingalignWidevalues are coming from invidual blocks' support of wide alignment, rather than themes'.)Note
This PR doesn't touch code that's relevant for mobile apps (i.e.
.native.jsfiles), which introduced support for wide alignment only recently (in #24598). Moving forward, we should make sure that those files also usegetThemeSupports()rather thangetSettings()cc/ @geriuxTypes of changes
Refactor.
Checklist: