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

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

Merged
merged 13 commits into from Apr 16, 2018

Conversation

@gziolo
Member

gziolo commented Feb 13, 2018

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

This comment has been minimized.

Contributor

youknowriad commented Feb 13, 2018

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 gziolo referenced this pull request Feb 13, 2018

Merged

Declare and override autocompleters via filter #4609

3 of 3 tasks complete

@aduth aduth referenced this pull request Mar 1, 2018

Closed

Add More Menu Item Api #4484

3 of 3 tasks complete

@gziolo gziolo added the Extensibility label Mar 8, 2018

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

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

@gziolo

This comment has been minimized.

Member

gziolo commented Mar 12, 2018

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

@gziolo

This comment has been minimized.

Member

gziolo commented Mar 29, 2018

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.

@@ -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'

This comment has been minimized.

@gziolo

gziolo Mar 29, 2018

Member

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

@gziolo gziolo added this to the 2.6 milestone Mar 30, 2018

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

This comment has been minimized.

@youknowriad

youknowriad Mar 30, 2018

Contributor

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?

This comment has been minimized.

@gziolo

gziolo Mar 30, 2018

Member

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.

This comment has been minimized.

@youknowriad

youknowriad Mar 30, 2018

Contributor

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?

This comment has been minimized.

@gziolo

gziolo Mar 30, 2018

Member

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 gziolo referenced this pull request Mar 30, 2018

Merged

Framework: Upgrade React version to 16.3.0 #5900

3 of 3 tasks complete
@mcsf

This comment has been minimized.

Contributor

mcsf commented Apr 2, 2018

I like where this is going. It feels like a good balance of magic, and the reason why it doesn't go overboard is that the abstraction itself makes sense: being in a context of block editing is something that makes sense for the block developer and for the user, and the idea that BlockControls et al. should be dependent on this context makes total sense.

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.

👍

@zgordon

This comment has been minimized.

zgordon commented Apr 2, 2018

I like this continued direction towards automagical and the notice in the console is something most would pick up. Nice to not be breaking change for those who started using isSelected 👍

@gziolo gziolo modified the milestones: 2.6, 2.7 Apr 5, 2018

aria-multiline="false"
class="wp-block-paragraph blocks-rich-text__tinymce"
contenteditable="true"
data-is-placeholder-visible="false"

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

This is very unexpected. It looks like context provider doesn't work properly with Enzyme, but I can't observe the same in the browser. Very annoying thing :(

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

It was code issue on our side :) Fixed with: fd740aa.

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 12, 2018

@youknowriad I addressed your comment related to RichText. It should work as expected without isSelected prop passed. I still need to add deprecation message to inform plugin developers about changes introduced.

Enzyme update is necessary to make tests work properly. I had to skip some tests and one of the tests behaves strangely (the one for Paragraph).

*/
import { ifBlockEditSelected } from '../block-edit/context';
const Fill = createSlotFill( 'BlockControls' );

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

I have an improvement for this one in another PR:
https://github.com/WordPress/gutenberg/pull/6086/files#diff-f55821c6d9b2038d9e462f87c1c9e0feR6

I will extract and update this PR if it is going to be ready to merge sooner.

const { Fill, Slot } = createSlotFill( 'BlockControls' );
*
* @return {Component} Component with a BlockEdit context set.
*/
export const withBlockEditContextProvider = createHigherOrderComponent( ( OriginalComponent ) => {

This comment has been minimized.

@youknowriad

youknowriad Apr 13, 2018

Contributor

I don't like the fact that Provider is included using a HoC instead of a regular Component using children. Because it gives the impression that we could use this component in several places where in reality the providers are used only once.

Thoughts?

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

Good point made :)
I will move it to BlockEdit.

Because it gives the impression that we could use this component in several places where in reality the providers are used only once.

Well, not in every case, but in general I agree 👍

@youknowriad

LGTM 👍 I believe a second opinion would be good here

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 13, 2018

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

screen shot 2018-04-13 at 15 30 00

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 13, 2018

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?

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

This comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

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

This comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Apr 16, 2018

Member

Much better: fbb66c8

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

This comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

I like this 👍

@@ -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.

This comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

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?

This comment has been minimized.

@gziolo

gziolo Apr 16, 2018

Member

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.

@@ -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 />

This comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

Glad we're losing the magic strings 👍

@@ -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 comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

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.

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

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

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

This comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

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?

This comment has been minimized.

@youknowriad

youknowriad Apr 13, 2018

Contributor

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

This comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

Confusing (two tiers of isSelected), but okay 😄

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

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.

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

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

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Apr 13, 2018

Member

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.

This comment has been minimized.

@jayshenk

jayshenk Apr 14, 2018

Contributor

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

This comment has been minimized.

@gziolo

gziolo Apr 16, 2018

Member

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.

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

This comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Apr 13, 2018

Member

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

This comment has been minimized.

@gziolo

gziolo Apr 16, 2018

Member

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 :)

@jayshenk jayshenk referenced this pull request Apr 13, 2018

Closed

Add Inline Images and Inline Blocks API #5794

4 of 4 tasks complete

@gziolo gziolo merged commit faabea1 into master Apr 16, 2018

2 checks passed

codecov/project 44.52% (+0.17%) compared to 0de347f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

@gziolo gziolo deleted the try/react-16.3-context branch Apr 16, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 16, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment