Move the editor settings into the editor's store #6500

youknowriad · 2018-04-30T11:54:39Z

In this PR I'm trying to get rid of withEditorSettings and just use the editor's store to hold these settings. It doesn't make sense to use another context provider for these while the editor store behaves in the exact same way.

youknowriad · 2018-04-30T11:55:18Z

blocks/block-alignment-toolbar/index.js

-		wideControlsEnabled: settings.alignWide,
+export default withSelect(
+	( select ) => ( {
+		wideControlsEnabled: select( 'core/editor' ).getEditorSettings().alignWide,


Targeted selectors like doesEditorSuppoortWideAlignment could be added later.

What about having an option to enabling this support to the existing selector?

wideControlsEnabled: select( 'core/editor' ).getEditorSettings( 'alignWide' ), // or singular wideControlsEnabled: select( 'core/editor' ).getEditorSetting( 'alignWide' ),

Yes, why not 👍 I'll leave those to a separate PR because this one is already big enough :)

youknowriad · 2018-04-30T11:56:17Z

editor/components/inserter-with-shortcuts/index.js

 		return {
+			items: getFrecentInserterItems( allowedBlockTypes, 4 ),


Since it's in state now, we could improve this selector to avoid the allowedBlockTypes argument.

youknowriad · 2018-04-30T11:57:39Z

blocks/block-alignment-toolbar/index.js

- * Internal dependencies
- */
-import { withEditorSettings } from '../editor-settings';
+import { withSelect } from '@wordpress/data';


Several components in the blocks module make use of these settings. These components should be moved to the editor module (related #6275) before merging this PR to ensure select( 'core/editor' ) is available.

noisysocks · 2018-05-07T01:15:59Z

Nice! This will solve a few pain points I've seen (e.g. #6067 cc. @brandonpayton)

Why use React/Redux to manage this data at all, though? Editor settings don't change throughout the lifetime of the application, so they could just as well be stored in a constant somewhere e.g. wp.editor.settings = { ... }.

noisysocks · 2018-05-07T01:17:12Z

blocks/editor-settings/index.js

-
-export default EditorSettings;
-
-export const withEditorSettings = ( mapSettingsToProps ) => createHigherOrderComponent(


Do we need to deprecate this for a few versions? I note that it's public via @wordpress/blocks.

Good point, I'll wrangle something

youknowriad · 2018-05-07T08:13:41Z

Why use React/Redux to manage this data at all, though? Editor settings don't change throughout the lifetime of the application, so they could just as well be stored in a constant somewhere e.g. wp.editor.settings = { ... }.

Why shouldn't they change? I mean if we built our application to be reactive to change to editor settings, that's a win?

And we do have a setting that can change: hasFixedToolbar.

brandonpayton · 2018-05-07T16:21:11Z

I am a big fan of this because it gives us a single place to reason about and work with editor state.

noisysocks · 2018-05-07T22:37:01Z

Why shouldn't they change? I mean if we built our application to be reactive to change to editor settings, that's a win?

Fair point 🙂

And we do have a setting that can change: hasFixedToolbar.

We currently fake this by overriding the setting value in <Editor> with isFeatureActive( 'fixedToolbar' ). If we go ahead with this approach, perhaps it would be worth refactoring this so that the actual editor setting itself changes.

youknowriad · 2018-05-08T07:33:00Z

We currently fake this by overriding the setting value in with isFeatureActive( 'fixedToolbar' ).

Not certain I agree, we do change the editorSettings which is a concept of the editor module. But we keep also keep track of the value as a preference in the edit-post for persistence.

jorgefilipecosta · 2018-05-08T14:56:37Z

core-blocks/image/block.js

-	} ),
-] )( ImageBlock );
+	return {
+		settings: select( 'core/editor' ).getEditorSettings(),


Minor: This change assume settings can now be dynamic, so we will re-render each time settings change, we just use maxWidth from settings so maybe we can just pass maxWidth now instead of all the settings.

Good point, though there's not real change compared to the previous version in term of rerendering. The previous version, the settings were also dynamic since it was using the new context API.

jorgefilipecosta · 2018-05-08T15:33:59Z

editor/store/reducer.js

+ *  blockTypes        boolean|Array  Allowed block types
+ *  hasFixedToolbar   boolean        Whether or not the editor toolbar is fixed
+ */
+const DEFAULT_SETTINGS = {


Maybe we should export this constant in './defaults'. Given that we have already had a file for defaults.

jorgefilipecosta · 2018-05-08T16:10:37Z

editor/store/effects.js

-	SETUP_EDITOR( action ) {
-		const { post, settings } = action;
+	SETUP_EDITOR( action, { getState } ) {
+		const { post } = action;


I think settings was part of the setup editor action just because of this effect. And now we are not using it anymore or passing it, so In my opinion, the settings parameter should be removed from setupEditor action.

jorgefilipecosta · 2018-05-08T19:00:43Z

editor/components/provider/index.js

+	}
+
+	componentDidUpdate( prevProps ) {
+		if ( this.props.settings !== prevProps.settings ) {


I really like this approach, the user changes a preference in the edit post store. That preference changes the props passed to the editor and the editor updates its configuration. I think this is the right way to communicate between stores 👍
I think we will probably have more uses of this pattern in the future.
The problem we may have is that when settings are updated a rerender happens, then componentDidUpdate is executed, we change the store and we may do another rerender after the store is changed. In this case, I think it is not problematic as we don't depend on the settings of the store, so we don't have double rerenders.
But, I would like to know what are your thoughts on this alternative approach as in other situations the prop may change frequently:
Add a withSelect that selects the current settings from the store, so we have currentSettings (old settings) and settings. Use getDerivedStateFromProps to compare both settings and if they are different call props.updateEditorSettings. In this case, we are updating the store before a re-render but it sounds like a hack. I'm not certain of what would be the correct solution for this pattern.

The problem with getDerivedStateFromProps is that it's a static function with no access to the previous props.

Yes, there's a potential double rendering when we use componentDidUpdate but there's nothing noticeable for the end user because even if React calls the DOM update methods, the UI won't render until the next JavaScript event loop item which is not called until the double rerendering finishes (Learned this from Dan Abramov's tweets :P )

jorgefilipecosta · 2018-05-08T19:02:28Z

I really like the changes we have here as it moves us further in the direction of having all data available via withSelect. In my tests, this seems to be working well 👍 Awesome work!

youknowriad · 2018-05-10T07:18:45Z

Thanks all for your feedback, I think I've addressed all the feedback here, so moving forward soon.

youknowriad commented Apr 30, 2018

View reviewed changes

youknowriad force-pushed the try/editor-settings-in-store branch from 4c2af3d to 4db91af Compare April 30, 2018 11:59

This was referenced May 4, 2018

Framework: Move components from the blocks to the editor module #6521

Merged

Rethink the separation between editor and blocks module #6275

Closed

Add withAllowedBlockTypes HOC and simplify existing code using it #6591

Closed

jorgefilipecosta self-requested a review May 4, 2018 15:02

noisysocks reviewed May 7, 2018

View reviewed changes

youknowriad force-pushed the try/editor-settings-in-store branch from 4db91af to 42485b5 Compare May 7, 2018 14:36

youknowriad mentioned this pull request May 7, 2018

[WIP] Child blocks #6346

Closed

4 tasks

youknowriad force-pushed the try/editor-settings-in-store branch from f051ab5 to 03ec2c1 Compare May 7, 2018 16:24

jorgefilipecosta reviewed May 8, 2018

View reviewed changes

youknowriad added 5 commits May 9, 2018 13:16

Move the editor settings into the editor's store

c0fb58a

Add a deprecation message for withEditorSettings

68af3fe

Fix unit tests

7621961

move editor settings defaults to the editor defaults file

b0a537b

Remove settings arg in the SETUP_EDITOR action

bb5cfe2

youknowriad force-pushed the try/editor-settings-in-store branch from 03ec2c1 to bb5cfe2 Compare May 9, 2018 12:22

youknowriad merged commit 63abd32 into master May 10, 2018

youknowriad deleted the try/editor-settings-in-store branch May 10, 2018 08:02

brandonpayton mentioned this pull request May 15, 2018

Add shared blocks to the blocks autocompleter #6067

Merged

3 tasks

danielbachhuber added this to the 2.9 milestone May 21, 2018

danielbachhuber mentioned this pull request May 21, 2018

Post formats not selectable #6864

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the editor settings into the editor's store #6500

Move the editor settings into the editor's store #6500

youknowriad commented Apr 30, 2018

youknowriad Apr 30, 2018 •

edited

Loading

gziolo Apr 30, 2018 •

edited

Loading

youknowriad Apr 30, 2018

youknowriad Apr 30, 2018

gziolo Apr 30, 2018

youknowriad Apr 30, 2018

noisysocks commented May 7, 2018

noisysocks May 7, 2018

youknowriad May 7, 2018

youknowriad commented May 7, 2018

brandonpayton commented May 7, 2018

noisysocks commented May 7, 2018

youknowriad commented May 8, 2018

jorgefilipecosta May 8, 2018

youknowriad May 8, 2018

jorgefilipecosta May 8, 2018 •

edited

Loading

jorgefilipecosta May 8, 2018

jorgefilipecosta May 8, 2018

youknowriad May 9, 2018 •

edited

Loading

jorgefilipecosta commented May 8, 2018 •

edited

Loading

youknowriad commented May 10, 2018

		return {
		items: getFrecentInserterItems( allowedBlockTypes, 4 ),


		export default EditorSettings;

		export const withEditorSettings = ( mapSettingsToProps ) => createHigherOrderComponent(

Move the editor settings into the editor's store #6500

Move the editor settings into the editor's store #6500

Conversation

youknowriad commented Apr 30, 2018

youknowriad Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

gziolo Apr 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisysocks commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented May 7, 2018

brandonpayton commented May 7, 2018

noisysocks commented May 7, 2018

youknowriad commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad May 9, 2018 • edited Loading

Choose a reason for hiding this comment

jorgefilipecosta commented May 8, 2018 • edited Loading

youknowriad commented May 10, 2018

youknowriad Apr 30, 2018 •

edited

Loading

gziolo Apr 30, 2018 •

edited

Loading

jorgefilipecosta May 8, 2018 •

edited

Loading

youknowriad May 9, 2018 •

edited

Loading

jorgefilipecosta commented May 8, 2018 •

edited

Loading