Migrate post editor editorMode to use preferences store and remove localAutosaveInterval preference
#39180
Conversation
|
Size Change: -105 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
I've stumbled on some compatibility issues with the mobile codebase in this PR, so I've pinged some mobile devs for a review. |
| dispatch( preferencesStore ).setDefaults( 'core/edit-post', { | ||
| editorMode: 'visual', | ||
| fixedToolbar: false, | ||
| fullscreenMode: true, | ||
| hiddenBlockTypes: [], | ||
| welcomeGuide: true, | ||
| } ); |
There was a problem hiding this comment.
I see there's some defaults here - https://github.com/WordPress/gutenberg/blob/trunk/packages/react-native-bridge/common/gutenberg-web-single-block/local-storage-overrides.json.
There are some preferences that don't have matching settings in the web codebase ('pinnedPluginItems', 'isGeneralSidebarDismissed' and 'showInserterHelpPanel'), so I'm not sure if those are mobile only features, or things that were removed in the web codebase.
The web codebase also stopped using 'nux' a long time ago. Is it still in use on mobile?
|
👋 In relation to the mobile unit tests that are failing, I've just created this PR to address the issue. Once it's merged and this branch is updated, we should see the |
talldan
force-pushed
the
migrate/post-editor-save-interval-and-mode-data
branch
from
82baf26
to
6bf209e
Compare
talldan
added
[Package] Edit Post
talldan
force-pushed
the
migrate/post-editor-save-interval-and-mode-data
branch
from
a4aee9a
to
5c94254
Compare
|
This should be ready for review now. |
|
Huh, why is |
There was a problem hiding this comment.
This is looking good—nice work!
It's surprising to me that localAutosaveInterval is a preference stored in localStorage instead of an editor setting passed to wp.editPost.initialize (and then subsequently wp.editor.EditorProvider).
Turns out it already is an editor setting. LocalAutosaveMonitor will read from __experimentalLocalAutosaveInterval.
gutenberg/packages/editor/src/components/local-autosave-monitor/index.js
Lines 180 to 181 in b7f0444
It's just that this editor setting comes from a preference.
gutenberg/packages/edit-post/src/editor.js
Lines 95 to 97 in b7f0444
From a git blame, it looks like this was implemented at the last moment in 64fac7e as a way to speed up E2E tests.
But we don't need to use a preference to have E2E tests change the timeout. We can simply dispatch updateEditorSettings in @wordpress/editor. This is already done for the autosave interval.
gutenberg/packages/e2e-tests/specs/performance/post-editor.test.js
Lines 92 to 94 in b7f0444
The default timeout of 15 can be added to to EDITOR_SETTINGS_DEFAULTS.
Note that it's a @wordpress/editor setting not a @wordpress/block-editor setting, as LocalAutosaveMonitor exists in @wordpress/editor.
So, I think let's remove the localAutosaveInterval preference altogether. We may as well also remove __experimental from __experimentalLocalAutosaveInterval while we're at it as it's clearly not going anywhere.
(For extra credit, it would be cool to make it so that an interval of -1 (or 0, or null, whatever!) disables the functionality so that we don't have to use 100000000000.)
packages/edit-post/src/editor.js
Outdated
| @@ -87,12 +89,13 @@ function Editor( { | |||
| focusMode: isFeatureActive( 'focusMode' ), | |||
| hasReducedUI: isFeatureActive( 'reducedUI' ), | |||
| hasThemeStyles: isFeatureActive( 'themeStyles' ), | |||
| preferredStyleVariations: getPreference( | |||
| preferredStyleVariations: getEditorPreference( | |||
| 'preferredStyleVariations' | |||
| ), | |||
| hiddenBlockTypes: getHiddenBlockTypes(), | |||
| blockTypes: getBlockTypes(), | |||
| __experimentalLocalAutosaveInterval: getPreference( | |||
There was a problem hiding this comment.
Why aren't we using getEditorPreference here?
There was a problem hiding this comment.
I guess our medium term plan is to deprecate getPreference in @wordpress/edit-post, so, maybe the actual question is: why are we using getEditorPreference above? 😀
There was a problem hiding this comment.
I'm migrating preferredStyleVariations in a separate branch. For now they're still stored in the edit-post preferences. They're a bit more complicated, so definitely need to be done in an individual PR. 😅
When that (and panels) are done I'll be able to drop getEditorPreference and it'll become a lot cleaner. 👍
| return getPreference( state, 'editorMode', 'visual' ); | ||
| } | ||
| export const getEditorMode = createRegistrySelector( ( select ) => () => | ||
| select( preferencesStore ).get( 'core/edit-post', 'editorMode' ) ?? 'visual' |
There was a problem hiding this comment.
Isn't the ?? unnecessary because we're using setDefaults?
There was a problem hiding this comment.
Yeah, it is. I added it back so that the unit tests are more logical. I suppose the unit tests could call setDefaults though. 🤔
There was a problem hiding this comment.
Nah all good that makes sense 👍
Sounds good. I did a search on wp directory (https://www.wpdirectory.net/search/01FXRVZ03CDA0A926P95E7WV61), and it looks like there's one plugin that makes use of the local autosave interval, and it's using the editor settings to interact with it. |
talldan
changed the title
localAutosaveInterval and editorMode to use preferences storeeditorMode to use preferences store and remove localAutosaveInterval preference
|
I've made those changes.
I haven't done this bit, it isn't completely straightforward and I don't want to mess around with the AutosaveMonitor. Currently the component has a way to disable the interval, but that means to save immediately rather than don't save at all. Adding an additional layer on top of this would make the component pretty confusing (and it's already a little difficult to follow). It could do with a refactor. |
There was a problem hiding this comment.
Confirmed that local autosaving still works by typing in some content and watching my Session Storage tab in DevTools. Think we're good here! Nice work 👍
I pushed up some changes to the editor settings doc comment which I saw had autosaveInterval in there but not locsalAutosaveInterval.
Feel free to make that lib/compat change or not. Up to you.
lib/editor-settings.php
Outdated
| @@ -24,6 +24,8 @@ function gutenberg_extend_post_editor_settings( $settings ) { | |||
| $settings['defaultTemplatePartAreas'] = get_allowed_block_template_part_areas(); | |||
| } | |||
|
|
|||
| $settings['__experimentalLocalAutosaveInterval'] = 15; | |||
There was a problem hiding this comment.
It might be good while we're here (sorry lol) to split this hook up into different lib/compat files so that we know which settings supposed to be copied over into Core come release time.
// lib/compat/wordpress-5.9/block-editor-settings.php
add_filter( 'block_editor_settings_all', function( $settings ) {
$settings['__unstableEnableFullSiteEditingBlocks'] = current_theme_supports( 'block-templates' );
if ( wp_is_block_theme() ) {
$settings['defaultTemplatePartAreas'] = get_allowed_block_template_part_areas();
}
return $settings;
} );
// lib/compat/wordpress-6.0/block-editor-settings.php
add_filter( 'block_editor_settings_all', function( $settings ) {
$settings['__experimentalLocalAutosaveInterval'] = 15;
return $settings;
} );
|
Thanks for another review and for fixing that doc block! |
|
It looks like the image block e2e tests are failing because of a bug in WordPress core related to the media endpoint. I'll follow up on that, but it's unrelated to this change, so I'm merging. edit: trac ticket here - https://core.trac.wordpress.org/ticket/55367 |
|
🎉 nice work |
|
Hi folks 👋 I notice that this is PR breaks the 2022-03-12_18-42-15.mp4In fact, I think that all the blocks from It's not obvious to me if this is a regression or if there are some changes required on the contributor's part to make the experimental core blocks work given this PR? Could you advise here and I can file an issue if this is indeed a regression, thanks! 🙂 |
|
For additional context: This issue only appears when editing a post - when using the site editor, the |
|
@michalczaplinski Thanks for letting me know. That's not something that I'd expect to break, I'll look into it. |
|
Turned out it was a pretty simple issue, a new PHP files this PR introduced wasn't being loaded. The fix is here - #39415. |
Backports changes from the Gutenberg plugin. Original PR in Gutenberg: WordPress/gutenberg#39180. Props zieleadam, talldanwp. See #55505. git-svn-id: https://develop.svn.wordpress.org/trunk@53090 602fd350-edb4-49c9-b593-d223f7449a82
Backports changes from the Gutenberg plugin. Original PR in Gutenberg: WordPress/gutenberg#39180. Props zieleadam, talldanwp. See #55505. Built from https://develop.svn.wordpress.org/trunk@53090 git-svn-id: http://core.svn.wordpress.org/trunk@52679 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Backports changes from the Gutenberg plugin. Original PR in Gutenberg: WordPress/gutenberg#39180. Props zieleadam, talldanwp. See #55505. Built from https://develop.svn.wordpress.org/trunk@53090 git-svn-id: https://core.svn.wordpress.org/trunk@52679 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Description
Addresses some of #31965
This migrates the post editor's
editorModepreferences to use the new preferences store. This preference is. used for determining whether the visual or code editor is shown.localAutosaveIntervalhas also been changed via some code review feedback. It's now now no longer an editor preference, instead this value is set or updated via editor settings alone. The setting is now stable, it's been around for a few years, so seems like a good time to make this happen.Testing Instructions
Code editor
trunkopen the post editor and dismiss the welcome guideAutosave Interval
No need to switch branches for this test, just build this branch.
Another option is to add a console log to this line - packages/editor/src/components/local-autosave-monitor/index.js to log out the value of
localAutosaveInterval. It should be15by default.Checklist:
*.native.jsfiles for terms that need renaming or removal).