Blocks: Try new context API to simplify display of block controls #5029

gziolo · 2018-02-13T12:49:18Z

Description

This PR updated React version to version 16.3.x (once it is stable we can update 15cd537 and move to its own PR). One of the new features that 16.3 is going to introduce is new context API. I gave it a spin to simplify the way we handle <InspectorControls /> and <BlockContols /> inside edit component included in every block.

Before

function edit( { isSelected } ) {
    return [
        isSelected && (
            <BlockControls key="controls">
                ...
            </BlockControls>
        ),
        isSelected && (
           <InspectorControls key="inspector">
                ...
            </InspectorControls>
        ),
        <RichText
                tagName="p"
                isSelected={ isSelected }
             ...
         />,
    ];
}

After

function edit( { isSelected } ) {
    return [
        <BlockControls key="controls">
            ...
        </BlockControls>,
        <InspectorControls key="inspector">
            ...
        </InspectorControls>,
        <RichText
                tagName="p"
             ...
         />,
    ];
}

Reasoning

I never liked that we need to prepend those controls with the check for every block. It is very easy to forget about it and it leads to unexpected behaviors. It turns out that this is a quite common issue that people have when learning how to create a new block. This is what I understood when talking to @zgordon. He shared also an example issue where the current logic caused troubles: Duplicate Inspector Controls.

Implementation

This PR wraps <EditBlock /> component, which renders edit component for each block, with its own context which stores isSelected value inside a consumer for later use. Both controls (<InspectorControls /> and <BlockContols />) get wrapped with a context consumer which renders them only when isSelected is truthy. As simple as that.
The same technique was used for <RichText /> component and its < BlockFormatControls /> after suggestion from @youknowriad .

How Has This Been Tested?

Manually tested if there were no regression. The only block that was upgraded so far is <Paragraph /> to use this new context. If we agree that this is the change we want, I'm happy to update all existing blocks (as a follow-up) ~~and the documentation~~ (docs updated).

The best way to test is to add a few paragraphs and make sure the display only their own controls.

I also made sure all unit tests pass (npm run test) and e2e tests pass on Travis.

TODO

Add deprecation notice to inform that isSelected is no longer mandatory.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

youknowriad · 2018-02-13T14:19:05Z

I really like the idea, this change in addition to the removal of the focus management will make block author's life easier.

I'm always suspicious when it comes to context usage in React in general but I think this is a good usage for it. BlockControls and InspectorControls slots are already magic, so adding a context value to simplify their usage makes sense to me especially since this check is consistent across all blocks.

gziolo · 2018-03-12T10:46:00Z

I figured out that build is failing because Enzyme doesn't support new tags created by createContext. See related issue: enzymejs/enzyme#1509 and PR: enzymejs/enzyme#1513.

gziolo · 2018-03-29T07:53:09Z

Updated with the recent changes in master and updated to React 16.3.0-rc.0.

Should we also do a similar trick for <RichText />? In that case, it might require a more sophisticated logic because in some places we pass some overrides based on other conditions.

gziolo · 2018-03-29T07:54:02Z

lib/client-assets.php

@@ -379,16 +379,16 @@ function gutenberg_register_vendor_scripts() {

 	gutenberg_register_vendor_script(
 		'react',
-		'https://unpkg.com/react@16.2.0/umd/react' . $react_suffix . '.js'
+		'https://unpkg.com/react@16.3.0-alpha.1/umd/react' . $react_suffix . '.js'


I missed that, it needs to be updated...

youknowriad · 2018-03-30T08:35:11Z

blocks/block-controls/index.js

+ */
+import { ifEditBlockSelected } from '../block-edit/context';
+
+export function BlockControls( { controls, children } ) {


How can we inform the user about this backward compatibity change. I think it's fine to ship it because users are probably already testing isSelected on their side but how can we inform them that it's not useful anymore?

It isn't a breaking change, so it's more about the convenience of developers and assuring that they don't introduce subtle bug as noted in here. I don't think we can add a deprecation message in this case as it's passed down in case of BlockControls and InspectorControls.

I was thinking maybe not a warning but we add a log message like this when registering custom blocks:

In this new Gutenberg Release, The `BlockControls`, `InspectorControls` and `RichText` automatically hides when the block is not selected, you don't need to check or pass the `isSelected` prop anymore.

And just leave this message for one release at least? What do you think?

Yes, I like this idea. I didn't think of it this way :)

youknowriad

I believe we should push this further and update the default value of the isSelected prop of the RichText component to be true now and wrap the Formatting Toolbar of the RichText component with this new HoC as well.

gziolo · 2018-04-13T13:31:10Z

Added message on the JS console to let developer know about changes introduced:

gziolo · 2018-04-13T13:50:57Z

LGTM 👍 I believe a second opinion would be good here

@youknowriad thanks for the review. @mcsf or @aduth would you mind confirming it is good to go?

aduth · 2018-04-13T14:18:52Z

blocks/block-edit/index.js


 export class BlockEdit extends Component {
+	constructor( props ) {
+		super( props );
+		this.state = {


I don't think a component needs both the initial setting of state and static getDerivedStateFromProps. The latter supersedes the need for the former. The constructor could be dropped altogether.

https://codepen.io/aduth/pen/QmRZLO

Edit: That said, it appears to be needed if you plan to return null; from static getDerivedStateFromProps, since otherwise the initial value of this.state is undefined.

https://codepen.io/aduth/pen/NYVOPa

Yes, I also expected you can defer creating the initial state, but it complains that it is undefined.

Since getDerivedStateFromProps is always called on the initial mount, do we really need to set the values of state in the constructor? Or can we just set it to an empty object?

https://codepen.io/aduth/pen/LdoKGW

Yes, let's do that. We will probably have to use get to check state, but in overall it's still better.

Much better: fbb66c8

aduth · 2018-04-13T14:23:23Z

blocks/block-edit/context.js

+ *
+ * @return {Component} Component which renders only when the BlockEdit is selected.
+ */
+export const ifBlockEditSelected = createHigherOrderComponent( ( OriginalComponent ) => {


I like this 👍

aduth · 2018-04-13T14:25:29Z

edit-post/index.js

@@ -57,6 +57,15 @@ export function initializeEditor( id, post, settings ) {
 	const target = document.getElementById( id );
 	const reboot = reinitializeEditor.bind( null, target, settings );

+	if ( 'production' !== process.env.NODE_ENV ) {
+		// Remove with 3.0 release.


I wonder if we could implement this logging with an object property getter on a block's this.props.isSelected, so we're not feeling compelled to remove it soon, and to not log when the message is not applicable (could be confusing, since a developer might assume there's something actively wrong with a block, even if there's not).

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

isSelected stays there, it is still useful in some other cases. Any ideas how to phrase it to ensure developers that their code will still work, but it can be simplified? Should we assume that updated docs are enough?

My main concern is that often this is used as follows:

isSelected && ( <InspectorControls key="inspector"> ... </InspectorControls> )

but also appears in a different context:

{ images.map( ( img, index ) => ( <li className="blocks-gallery-item" key={ img.id || img.url }> <GalleryImage url={ img.url } alt={ img.alt } id={ img.id } isSelected={ isSelected && this.state.selectedImage === index } onRemove={ this.onRemoveImage( index ) } onSelect={ this.onSelectImage( index ) } setAttributes={ ( attrs ) => this.setImageAttributes( index, attrs ) } caption={ img.caption } /> </li> ) ) }

It is very hard to handle it on a case by case basis.

aduth · 2018-04-13T14:26:17Z

editor/components/block-toolbar/index.js

@@ -18,8 +18,8 @@ function BlockToolbar( { block, mode } ) {
 	return (
 		<div className="editor-block-toolbar">
 			<BlockSwitcher uids={ [ block.uid ] } />
-			<Slot name="Block.Toolbar" />
-			<Slot name="Formatting.Toolbar" />
+			<BlockControls.Slot />


Glad we're losing the magic strings 👍

aduth · 2018-04-13T14:27:51Z

docs/deprecated.md

@@ -9,6 +9,7 @@ Gutenberg's deprecation policy is intended to support backwards-compatibility fo
 ## 2.7.0

 - `wp.element.getWrapperDisplayName` function removed. Please use `wp.element.createHigherOrderComponent` instead.
+- `isSelected` usage is no longer mandatory with `BlockControls`, `InspectorControls` and `RichText`. It is now handled by the editor internally to ensure that controls are visible only when block is selected. See updated docs: https://github.com/WordPress/gutenberg/blob/master/blocks/README.md#components.


This is a bit out of place; the document is grouped by versions where specific features are removed, as a hint to developers to ensure updating in time. In this case, there's nothing backwards-incompatible.

Well, yes. It's tricky. I will remove and ask @mtias to include in the release post.

aduth · 2018-04-13T14:28:47Z

blocks/rich-text/index.js

-					</Fill>
-				}
-				{ isSelected && inlineToolbar &&
+				{ isSelected && ! inlineToolbar && (


Is isSelected here redundant? Since BlockFormatControls is already wrapped with ifBlockEditSelected.

Edit: I see we have specific handling of isSelected in this component. What's the use case for that?

The usecase is for multiple RichText blocks (quote for example)

Confusing (two tiers of isSelected), but okay 😄

Yes, multiple RichText is quite difficult to handle and maintain. I will look into it next week. Maybe it wouldn't be that hard to automatically handle active state in a way where only one RichText can be active per block at a time.

If we would have isBlockSelected and isRichTextSelected it would be easier to read.

Maybe it wouldn't be that hard to automatically handle active state in a way where only one RichText can be active per block at a time.

I think we can have only one active RichText for the whole post. Maybe we can just use a component id and store id of RichText that is active, it would simplify the logic of some blocks.

I like the idea of storing a single active RichText in the editor store, if this is what you mean. It would be useful for #5794, so that inline images could be inserted anywhere RichText is used (not just in the Paragraph block), and so that the global inserter could select the state of the active RichText in the editor store when determining whether to render the Inline Blocks menu. CC @nb

I think we can have only one active RichText for the whole post. Maybe we can just use a component id and store id of RichText that is active, it would simplify the logic of some blocks.

Yes, that's true. In addition, there might be only one active RichText per selected block. So I suppose we can reuse Context API for the same purpose and store the unique id of the actually active RichText component to make sure we don't display controls for the remaining components.

aduth · 2018-04-13T14:31:26Z

blocks/library/paragraph/index.js

@@ -224,8 +226,6 @@ class ParagraphBlock extends Component {
 						/>
 					</PanelBody>
 				</InspectorControls>
-			),
-			<div key="editable">


This appears useless.. but are we sure it was?

I can revert, but I didn't discover any issues so far in my testing. There are 5-ish more div wrappers there 🤣

I will revert this particular change to be on the safe side, but I will try to remove it anyway in a separate PR when tackling this nested divs hell :)

…introduced

…dled

…k is selected This makes it backward compatible with the existing core blocks

…Selected changes

gziolo · 2018-04-16T09:47:20Z

Moving forward with this one. There is no easy way to provide a message to the developers that isSelected in some cases is optional that's why I proceeded with the console.info approach. I will share on Twitter and #core-editor channel about this change. That's the only thing we can do.

gziolo added Framework Issues related to broader framework topics, especially as it relates to javascript [Feature] Blocks Overall functionality of blocks labels Feb 13, 2018

gziolo self-assigned this Feb 13, 2018

gziolo added the [Status] In Progress Tracking issues with work in progress label Feb 13, 2018

gziolo requested review from youknowriad, aduth, mcsf, jorgefilipecosta and ellatrix February 13, 2018 12:49

gziolo mentioned this pull request Feb 13, 2018

Declare and override autocompleters via filter #4609

Merged

3 tasks

aduth mentioned this pull request Mar 1, 2018

Add More Menu Item Api #4484

Closed

3 tasks

gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Mar 8, 2018

gziolo added this to To do in Extensibility via automation Mar 8, 2018

gziolo moved this from To do to In progress in Extensibility Mar 8, 2018

gziolo force-pushed the try/react-16.3-context branch 4 times, most recently from f8fa7f0 to 44b9536 Compare March 12, 2018 10:32

gziolo removed the [Status] In Progress Tracking issues with work in progress label Mar 12, 2018

gziolo force-pushed the try/react-16.3-context branch from 44b9536 to b5a77d2 Compare March 29, 2018 07:46

gziolo commented Mar 29, 2018

View reviewed changes

gziolo force-pushed the try/react-16.3-context branch 2 times, most recently from 475038b to 34f68ee Compare March 30, 2018 07:50

gziolo added this to the 2.6 milestone Mar 30, 2018

gziolo force-pushed the try/react-16.3-context branch from 34f68ee to 3192ba7 Compare March 30, 2018 08:20

youknowriad reviewed Mar 30, 2018

View reviewed changes

aduth reviewed Apr 13, 2018

View reviewed changes

jayshenk mentioned this pull request Apr 13, 2018

Add Inline Images and Inline Blocks API #5794

Closed

4 tasks

gziolo added 13 commits April 16, 2018 07:47

Edit Post: Add information for developers about isSelected changes …

966dab4

…introduced

Blocks: Refactor block controls to use block edit context

b89bd58

Docs: Update docs to reflect the change in how block controls are han…

91c7185

…dled

Blocks: Add isSelected handling to InspectorAdvancedControls

9e37c97

Blocks: Replace getWrapperDisplayName with its new alternative

39b2561

Blocks: Use createSlotFill for BlockControls

458b6f3

Blocks: Introduce BlockFormatControls component

4e4404a

Blocks: Make sure that RichText controls are only displayed when bloc…

c650c18

…k is selected This makes it backward compatible with the existing core blocks

Blocks: Update test helper to work with block edit context

8c95bb0

Blocks: Make sure that EditContextProvider is only rerendered when is…

33d8a56

…Selected changes

Blocks: Merge Provider with BlockEdit

f47a184

Blocks: Improve state handling for BlockEdit context

fbb66c8

Blocks: Addressed feedback related to BlockEditContext changes

cf63e10

gziolo force-pushed the try/react-16.3-context branch from c74fbcb to cf63e10 Compare April 16, 2018 06:39

gziolo merged commit faabea1 into master Apr 16, 2018

Extensibility automation moved this from In progress to Done Apr 16, 2018

gziolo deleted the try/react-16.3-context branch April 16, 2018 09:44

This was referenced Apr 16, 2018

Blocks: Remove obsolete isSelected usage (part 1) #6201

Merged

Blocks: Remove obsolete isSelected usage (part 2) #6248

Merged

Blocks: Remove obsolete isSelected usage (part 3) #6280

Merged

gziolo mentioned this pull request Apr 26, 2018

Blocks: Add automatic handling of focus for RichText component #6419

Merged

4 tasks

noisysocks mentioned this pull request May 2, 2018

Contact Form: Add Gutenberg based form builder blocks Automattic/jetpack#9452

Closed

aduth mentioned this pull request May 2, 2018

Rich Text: Deprecate event proxying #6286

Merged

aduth mentioned this pull request Jan 31, 2019

How are edit methods dissected when displaying the editor? #12227

Closed

snyk-bot mentioned this pull request Mar 25, 2021

[Snyk] Security upgrade stylelint from 9.10.1 to 13.8.0 wpscholar/gutenberg#22

Open

Blocks: Try new context API to simplify display of block controls #5029

Blocks: Try new context API to simplify display of block controls #5029

Conversation

gziolo commented Feb 13, 2018 • edited

Description

Before

After

Reasoning

Implementation

How Has This Been Tested?

TODO

Checklist:

youknowriad commented Feb 13, 2018

gziolo commented Mar 12, 2018 • edited

gziolo commented Mar 29, 2018

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

youknowriad left a comment

Choose a reason for hiding this comment

gziolo commented Apr 13, 2018

gziolo commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Apr 13, 2018 • edited

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

gziolo Apr 13, 2018 • edited

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

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

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

gziolo Apr 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Apr 16, 2018

gziolo commented Feb 13, 2018 •

edited

gziolo commented Mar 12, 2018 •

edited

aduth Apr 13, 2018 •

edited

gziolo Apr 13, 2018 •

edited

gziolo Apr 13, 2018 •

edited