Framework: Hide The Editor Settings from the block author

[youknowriad]

as a follow up to #2116 this hides the editor settings from the block author and in the same time it provider a nicer way to retrieve these settings from any component using the withEditorSettings HoC. This uses the same technique we use for the EditableProvider or ReduxProvider by storing the editor settings in the React Context.

An alternative would have been to rely on the Redux Store to store the editor settings, an alternative I tried in #2065. I have a feeling that these settings should be separate from the store because they don't change over time but not sure either.

These settings could store more than this, think:

• Color palettes provided by the theme
• Thumbnail sizes
• The blockTypes allowed in the current editor

This could be the starting point of avoiding to rely on the global registered blockTypes inside the editor.

[codecov]

Codecov Report

Merging #2119 into master will increase coverage by 1.72%.
The diff coverage is 17.64%.

@@            Coverage Diff             @@
##           master    #2119      +/-   ##
==========================================
+ Coverage   20.33%   22.05%   +1.72%     
==========================================
  Files         136      138       +2     
  Lines        4274     4326      +52     
  Branches      724      739      +15     
==========================================
+ Hits          869      954      +85     
+ Misses       2872     2843      -29     
+ Partials      533      529       -4

Impacted Files	Coverage Δ
blocks/library/cover-image/index.js	30.43% <ø> (ø)	⬆️
blocks/library/pullquote/index.js	33.33% <ø> (ø)	⬆️
blocks/library/gallery/index.js	25.71% <ø> (ø)	⬆️
editor/state.js	92.61% <ø> (-0.15%)	⬇️
blocks/library/image/index.js	15.62% <ø> (ø)	⬆️
editor/selectors.js	96.29% <ø> (-0.04%)	⬇️
blocks/library/table/index.js	36.36% <ø> (ø)	⬆️
blocks/library/cover-text/index.js	30% <ø> (ø)	⬆️
editor/settings/provider.js	0% <0%> (ø)
editor/index.js	0% <0%> (ø)	⬆️
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fef2904...e60e988. Read the comment docs.

[aduth]

One big caveat with context is that it does not play well with shouldComponentUpdate. This is discussed in #2108 and facebook/react#2517, which led to the EventEmitter usage you remarked about at #2108 (comment) . The result of the caveat is that parent -> descendant communication is very difficult to update. This doesn't seem so problematic here... yet. Since settings isn't something we expect to change over the lifetime of the application. But if this were something we were to want to support, it would be made much more difficult.

I'm also wondering just how far we can push the fake divide between Editable / blocks and the editor itself. With these changes, Editable has knowledge of a parent editor, so it makes me question whether the distinction is worth making or could at least be shuffled around, or if we might as well connect Editable to the editor state (I don't like the idea at first glance).

[youknowriad]

Yes, there's no big difference between using the context for the editor settings and using the context for the redux store and storing the editor settings in the store. The only difference is that the store already has the "EventEmitter" interface if we suppose that the settings could change, thus I'm not against using the store and just connecting components (I guess it's the same for the EditableProvider).

Also, I don't the think the distinction we have between editor/blocks like we have it now is a good one, I think the distinction introduced in #2065 is much more "logical".

• The editor is a generic configurable/reusable Block Editor. It provides "helpers" to ease building blocks: InspectorControls, BlockControls, Editable, AlignmentToolbars...

• A blocks-core module could contain the parsing, serializing (but not registration), and helpers.

• We could extract some blocks from the editor-chrome module to a generic content-blocks module. And these blocks should be independent of WP to make them reusable in any block editor without prerequisites

• We could extract the wp-blocks to their own module: The module creating the blocks that can work inside WordPress (need API, media modal...)

• The editor-chrome which is just and Application providing some blocks and using the editor generic module to build a full-featured post editor.

I see these modules as a longer term goal we could achieve step by step.

--
About the current PR, Do you think I should just use the Redux store instead of using a specific provider/HoC?

[youknowriad]

Ok so thinking more about this. In these components (Toolbars, controls...), we can't add a dependency to a store containing posts.... Unless we split the redux store (like done in #2065), we should keep this PR as is.

When splitting out the editor's state from the store, we could reconsider.

[youknowriad]

I'd like to use the work done here as a start to avoid using the globally registered block types, Any objection in merging? or maybe any recommendation on how to make this better?

[aduth]

I noted it in #2108: the react-side-context project is a neat agnostic "give-me-the-thing-in-context" / "i-have-the-thing-to-give" approach. Not sure if you think something like that could be useful, or if it encourages abuse of context.

I'm not so certain about all of the splitting you're proposing. In fact, I'm wondering if we should remove some of the splits more than I am considering making more. That is, blocks are very much related to the idea of an editor. It makes me uncomfortable to see withEditorSettings ("editor" specifically) only because we're pretending for them to be distinct things, when in fact they're pretty closely related.

In any case, this is a problem to be solved separately. The immediate effect of this removing settings concerns from the block implementer is a good one, and this gives us the flexibility to refactor in the future if we need 👍

[aduth]

Do we really need a mapping? Should we just pass all settings?

[youknowriad]

I'm planning to introduce the blockTypes, fallbackBlockName.... to these settings and I expect several components to extract a single or multiple blockTypes from the settings. Mapping helps use factorize this using functions similar to selectors to avoid duplication.

Also, It's nice that the component prop is wideAlignmentsEnabled instead of leaving the responsibility of extracting the prop to the component itself.

[aduth]

Do we need the nesting in settings. What else will live there? Maybe just blocks/with-editor-settings/index.js ?

[aduth]

I wonder if we can start thinking of a nicer way to compose all these providers...

[youknowriad]

yeah, maybe composeProviders or something :)

Maybe if we extract the editor, we could reduce the number of providers into one (inside the generic editor component) and provide an HoC like withEditorData instead of withEditorSettings to retrieve the editor settings/editor local state from any component. (just an idea we could explore later, not sure if it's a good one yet)

[youknowriad]

Thanks for the review, Andrew, really good notes here.

I noted it in #2108: the react-side-context project is a neat agnostic "give-me-the-thing-in-context" /

I missed the note, thanks for sharing here. I discover this library and I like it, I'll probably do some explorations with this lib later.

I'm not so certain about all of the splitting you're proposing. In fact, I'm wondering if we should remove some of the splits more than I am considering making more.

When it comes to module splitting, I think a "middle" approach is hard to do. I'd say the two options are:

• Splitting to small modules
• Gathering a lot of code in a big module

I think splitting into small modules will help us re-using these pieces in the Customization etc...
Also, I think it's easier to gather smaller modules than split a big interdependent one.