Codecov Report

Merging #3649 into master will decrease coverage by <.01%.
The diff coverage is 16.1%.

@@            Coverage Diff             @@
##           master    #3649      +/-   ##
==========================================
- Coverage   37.32%   37.31%   -0.01%     
==========================================
  Files         277      284       +7     
  Lines        6685     6692       +7     
  Branches     1213     1206       -7     
==========================================
+ Hits         2495     2497       +2     
- Misses       3532     3544      +12     
+ Partials      658      651       -7

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 9864a60...d8a28c2. Read the comment docs.

I took a quick glance at this code. Looks pretty good.

The "Show block inspector" button proved to be a challenge, because the idea is that clicking this button shows the inspector, but since the BlockInspector is a reusable component that can be shown anyware in the application using the "editor" module, we needed a showBlockInspector to allow the surrouding application (edit-post) to handle this button properly.

I'm not so happy about passing it down to all those components. I will think about some alternatives.

We discussed it extensively on Slack, but let me summarize my feedback here.

So as I understand, the idea here is to have isolated Redux state for the editor and a separate one for edit-post or any other type that we introduce in the future. It's not ideal because all contributors will have to pass the store name explicitly in the connect method. We might want to create local connect method and/or document it properly. We could add lint option to make sure editor library uses connect method properly.

I’m not happy about onShowInspector is connected at the UI level with Settings button:

It's passed down from edit-post's <Layout /> through several child components. All other toolbar buttons are handled within the editor, this one is not. I guess one of the reasons is because we can’t expose its current state to the edit-post library. Another one is as @youknowriad and @mtias suggested is that some apps might want to display advanced settings in a modal. I think the latter is a good reason to pass down this callback to make it easy to customize. I still hope we can find a solution that will simplify exposing this method to the nested components.

I was exploring if we could reuse pattern from <BlockCount /> to tackle isSidebarOpened flag differently. We could rename it to sth like areAdvancedSettingActive and still keep in the editor's state. We could expose it as <AdvancedSettings /> component and use this way inside edit-post library:

<AdvancedSettings>
        { ( areAdvancedSettingActive, closeAdvancedSettings ) => areAdvancedSettingActive && <Sidebar onClose={ closeAdvancedSettings } /> }
</AdvancedSettings>

This would solve the nesting issue, but the obvious issue here is that it won't play well with the modal case.

It would be nice to gather feedback from @mkaz and @georgeh how this proposal fits with their work on P2 integration. It would be a very valuable to hear from them before we proceed.

One more thing to consider. Previously only editor library was using Redux libraries. This PR changes that, which will cause both editor and edit-post to bundle the same files twice. We should either make those libs external or put them inside shared wp.store library similat to what wp.element does.

I'm closing this right now, I'll do another attempt once the "data" module finalized, which will help with state access accross modules.