New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes the nested blocks contextual toolbar preference #6082

Merged
merged 7 commits into from Apr 13, 2018

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Apr 9, 2018

closes #6047

On nested blocks, when we switch the "Fixed toolbar" preference, the UI was not being updated because the InnerBlocks component is cached and doesn't change on prop change.

So solve this issue, this does two things:

  • Move the hasFixedToolbar preference to the editor settings object to avoid having to pass it down as a prop to all the needed components and rely on context instead.

  • Since this preference can change over time, the old implementation of React context is not possible, so I'm also updating the editorSettings to rely on the new React context API.

This PR surfaces several things:

1 - The blocks module needs to know about the editorSettings (editor module). Previously, it was implicit because we we're just grabbing a context value, with the next React API, it's explicit (better IMO) because we need to import the Consumer component. In this first implementation, I implemented the EditorSettings context in the blocks module to avoid circular dependencies. But This is just one other thing proving that the distinction we have right now between blocks and editor is not correct and should be rethought separately.

2 - The new Editor Settings Object can change over time (it used to be immutable). This is already handled properly everywhere using the Context API. Aside one place, the setupEditor action is being triggered using the settings object. I think this is used to check the post against its template (which is available in the settings). I think it's fine for now because the template config can't be changed yet, but we could imagine rechecking again if the settings change. (retrigger a setupEditor action).

3 - Also this raises the question on the difference between the data module and the new React context API. And the response is simple, there's no difference conceptually :). So why I didn't use the data module instead of the React context API here? Because I didn't want to introduce a blocks store yet while the distinction between editor and blocks is not clear yet (see point 1). We should probably be using the editor store to hold these settings instead of a newly created blocks store. Bur for this to happen we need to merge these two modules first. So the tradeoff here is to use the React API and we'd be able to replace it easily with the data module once the point 1 is fixed.

Testing instructionos

  • Add a columns block
  • Switch the "Fixed toolbar preference"
  • The toolbar should show up at the right place depending on the value of the preference.

@youknowriad youknowriad self-assigned this Apr 9, 2018

@youknowriad youknowriad requested review from mcsf, gziolo and aduth Apr 9, 2018

@youknowriad youknowriad added this to the 2.7 milestone Apr 9, 2018

@aduth

A casual observer will have a difficult time distinguishing between EditorProvider and EditorSettings.Provider.

Will we have similar issues with renderBlockMenu ?

showContextualToolbar={ ! isLargeViewport || ! hasFixedToolbar }
renderBlockMenu={ VisualEditorBlockMenu }
/>
<BlockList renderBlockMenu={ VisualEditorBlockMenu } />
</ObserveTyping>
</WritingFlow>
</BlockSelectionClearer>
);
}
export default compose( [

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

We don't need compose anymore?

@@ -30,7 +30,7 @@ class BlockPreview extends Component {
// we inject this function via context.
return {
createInnerBlockList: ( uid ) => {
return createInnerBlockList( uid, noop, noop );
return createInnerBlockList( uid, noop );

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

Aside: Maybe these ought to be default values assigned from createInnerBlockList.

console.error( 'The Editor Provider Props are immutable.' );
this.store.dispatch(
setupEditor( props.post, {
...EditorSettings.defaultSettings,

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

Something about this feels off to me. Partly because we're duplicating the initialization of these settings between here and below. I think also it's odd that we don't need to observe changes in settings here; why does this only need to occur at setup time, and never thereafter? Should this be a proper EditorSettings.Consumer so we can just leverage the value from EditorSettings.Provider directly?

This comment has been minimized.

@youknowriad

youknowriad Apr 9, 2018

Contributor

Noted in the description :) (Second point)

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

Ah, I did read it, apparently not closely enough.

/**
* The default editor settings
* You can override any default settings when calling initializeEditor

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

This DocBlock feels outdated, particularly in its reference to initializeEditor of another module, and that we document only one of the many settings available (alignWide).

*
* alignWide boolean Enable/Disable Wide/Full Alignments
*
* @var {Object} DEFAULT_SETTINGS

This comment has been minimized.

@aduth

aduth Apr 9, 2018

Member

I don't know that @var is correct here, and I'm not sure why its value is repeated DEFAULT_SETTINGS.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 10, 2018

Conceptually renderBlockMenu suffers from the same issues but since it's immutable we don't have any bug. I'm on the fence on whether to add it to the editorSettings specially since it's not scalar. What do you think?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 12, 2018

Ok this should be ready, any concern?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 13, 2018

Granted that it's not perfect. (The initialization and renderBlockMenu). I'm getting this in as it's an improvement, it fixes an issue. And once we revisit the editor/blocks separation I'll revisit it.

@youknowriad youknowriad merged commit 10cf461 into master Apr 13, 2018

2 checks passed

codecov/project 44.42% (+0.04%) compared to 9c21156
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/fixed-toolbar-settings branch Apr 13, 2018

@@ -37,7 +37,7 @@ class BlockInsertionPoint extends Component {
}
}
export default compose(
withContext( 'editor' )( ( { templateLock } ) => ( { templateLock } ) ),
withEditorSettings( ( { templateLock } ) => ( { templateLock } ) ),
ifCondition( ( { templateLock } ) => ! templateLock ),

This comment has been minimized.

@aduth

aduth Apr 16, 2018

Member

Aside: Should we collapse these into a single higher-order component?

ifEditable = compose( withEditorSettings, ifCondition );
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment