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

Extensibility: Defer applying filters for component until it is about to be mounted #4002

Merged
merged 1 commit into from Dec 15, 2017

Conversation

Projects
None yet
5 participants
@gziolo
Member

gziolo commented Dec 14, 2017

Description

Tries to address the issue raised by @kmgalanakis and confirmed by @andreiglingeanu in #3800:

Correct me if I'm wrong, but it's also impossible to hook to the blocks.BlockEdit hook, after the last update (1.9)

This PR tries to postpone applying filters to all components wrapped with withFilters Higher-Order Component until the original component is about to be rendered. In most of the cases, it will happen when the editor is rendered. This should give enough flexibility to make adding filters straightforward.

I'm planning to explore if we could also rerender such components whenever filters get added or removed. It might be provided in its own PR.

How Has This Been Tested?

Manually, unit tests.

I tested it by loading Gutenberg and clicking around to make sure everything works without errors.

The best way is to register your own filter in the console which replaces the block and add new block to make sure it works:

wp.hooks.addFilter( 'blocks.BlockEdit', 'my/filters', () => () => wp.element.createElement( 'h1', {}, 'My block' ) );

You should see sth like:

screen shot 2017-12-14 at 14 50 37

Checklist:

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

@gziolo gziolo self-assigned this Dec 14, 2017

@gziolo gziolo requested review from aduth and youknowriad Dec 14, 2017

@andreiglingeanu

This comment has been minimized.

Show comment
Hide comment
@andreiglingeanu

andreiglingeanu Dec 14, 2017

Contributor

Looks awesome 👍

Contributor

andreiglingeanu commented Dec 14, 2017

Looks awesome 👍

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Dec 14, 2017

Indeed, this will help a lot! 👍

kmgalanakis commented Dec 14, 2017

Indeed, this will help a lot! 👍

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 14, 2017

Member

I'm planning to explore if we could also rerender such components whenever filters get added or removed. It might be provided in its own PR.

It looks like you can't subscribe for changes in the filters registry. I pinged @adamsilverstein on Slack to confirm.

Member

gziolo commented Dec 14, 2017

I'm planning to explore if we could also rerender such components whenever filters get added or removed. It might be provided in its own PR.

It looks like you can't subscribe for changes in the filters registry. I pinged @adamsilverstein on Slack to confirm.

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Dec 15, 2017

Contributor

@gziolo would something like this suffice in hooks to enable what you want to do? https://github.com/WordPress/packages/compare/feature/events-on-add-remove?expand=1

Contributor

adamsilverstein commented Dec 15, 2017

@gziolo would something like this suffice in hooks to enable what you want to do? https://github.com/WordPress/packages/compare/feature/events-on-add-remove?expand=1

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 15, 2017

Member

@adamsilverstein, thanks for sharing this. Yes, I think it might work. It's a pretty nice idea, anyway. Let's add this to hooks and see how it integrates here.

I'm merging this PR to make integration easier. I will open a follow-up once we have changes for hooks published to npm :)

Member

gziolo commented Dec 15, 2017

@adamsilverstein, thanks for sharing this. Yes, I think it might work. It's a pretty nice idea, anyway. Let's add this to hooks and see how it integrates here.

I'm merging this PR to make integration easier. I will open a follow-up once we have changes for hooks published to npm :)

@gziolo gziolo merged commit 0f35fc6 into master Dec 15, 2017

3 checks passed

codecov/project Absolute coverage decreased by -0.27% but relative coverage increased by +61.77% compared to 29b757d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gziolo gziolo deleted the update/defer-apply-filters-component branch Dec 15, 2017

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Jan 5, 2018

@gziolo I can confirm that BlockEdit is working fine!

I have a problem though doing a very simple thing. I want to add a simple button to the paragraph block. I've managed to do to but I get an error.

Here is my code

function addTestButton( BlockEdit ) {

    function extendParagraphBlock( props ) {
        var el = wp.element.createElement;
        var __ = wp.i18n.__;
        var BlockControls = wp.blocks.BlockControls;
        var Button = wp.components.Button;

        var element = el( BlockEdit, props );
        if ( 'core/paragraph' === props.name ) {

            function onTestButtonClick( props ) {
                alert('YEAH!');
                return;
            }

            function TestButton( props ) {
                const containerProperties = props.container ? props.container : {};

                const buttonProperties = props.button ? props.button : {};

                const buttonTitle = buttonProperties.title ? buttonProperties.title : 'My button';

                const button = wp.element.createElement( Button, buttonProperties, [ buttonTitle ] );

                const container = wp.element.createElement( 'div', containerProperties, [ button ] );

                return container;
            }

            var testButton = !! props.focus
                && el(
                    BlockControls,
                    { key: 'test_button_block' },
                    el(
                        TestButton,
                        {
                            button: {
                                key: 'test_button',
                                isPrimary: false,
                                title: __( 'Test', 'test' ),
                                onClick: onTestButtonClick
                            },
                            container: {
                                className: 'components-toolbar'
                            },
                        }
                    )
                );
            element = [ element, testButton ];
        }
        return element;
    }

    return extendParagraphBlock;
}
wp.hooks.addFilter( 'blocks.BlockEdit','test-block', addTestButton );

The error I'm getting is

Warning: Each child in an array or iterator should have a unique "key" prop. See https://fb.me/react-warning-keys for more information.
    in extendParagraphBlock (created by Filters(BlockEdit))
    in Filters(BlockEdit) (created by BlockListBlock)
    in BlockCrashBoundary (created by BlockListBlock)
    in div (created by BlockListBlock)
    in div (created by BlockListBlock)
    in BlockListBlock (created by Filters(BlockListBlock))
    in Filters(BlockListBlock) (created by WrappedComponent)
    in WrappedComponent (created by Connect(WrappedComponent))
    in Connect(WrappedComponent) (created by BlockList)
    in div (created by BlockList)
    in BlockList (created by Connect(BlockList))
    in Connect(BlockList) (created by VisualEditor)
    in div (created by WritingFlow)
    in WritingFlow (created by Connect(WritingFlow))
    in Connect(WritingFlow) (created by VisualEditor)
    in div (created by VisualEditor)
    in VisualEditor (created by Connect(VisualEditor))
    in Connect(VisualEditor) (created by Layout)
    in div (created by Layout)
    in div (created by Layout)
    in div (created by Layout)
    in Layout (created by _class)
    in div (created by _class)
    in _class (created by Connect(_class))
    in Connect(_class)
    in ErrorBoundary
    in Provider (created by EditorProvider)
    in EditableProvider (created by EditorProvider)
    in SlotFillProvider (created by EditorProvider)
    in APIProvider (created by EditorProvider)
    in DropZoneProvider (created by EditorProvider)
    in EditorProvider

Any ideas of what might be wrong with my code?

kmgalanakis commented Jan 5, 2018

@gziolo I can confirm that BlockEdit is working fine!

I have a problem though doing a very simple thing. I want to add a simple button to the paragraph block. I've managed to do to but I get an error.

Here is my code

function addTestButton( BlockEdit ) {

    function extendParagraphBlock( props ) {
        var el = wp.element.createElement;
        var __ = wp.i18n.__;
        var BlockControls = wp.blocks.BlockControls;
        var Button = wp.components.Button;

        var element = el( BlockEdit, props );
        if ( 'core/paragraph' === props.name ) {

            function onTestButtonClick( props ) {
                alert('YEAH!');
                return;
            }

            function TestButton( props ) {
                const containerProperties = props.container ? props.container : {};

                const buttonProperties = props.button ? props.button : {};

                const buttonTitle = buttonProperties.title ? buttonProperties.title : 'My button';

                const button = wp.element.createElement( Button, buttonProperties, [ buttonTitle ] );

                const container = wp.element.createElement( 'div', containerProperties, [ button ] );

                return container;
            }

            var testButton = !! props.focus
                && el(
                    BlockControls,
                    { key: 'test_button_block' },
                    el(
                        TestButton,
                        {
                            button: {
                                key: 'test_button',
                                isPrimary: false,
                                title: __( 'Test', 'test' ),
                                onClick: onTestButtonClick
                            },
                            container: {
                                className: 'components-toolbar'
                            },
                        }
                    )
                );
            element = [ element, testButton ];
        }
        return element;
    }

    return extendParagraphBlock;
}
wp.hooks.addFilter( 'blocks.BlockEdit','test-block', addTestButton );

The error I'm getting is

Warning: Each child in an array or iterator should have a unique "key" prop. See https://fb.me/react-warning-keys for more information.
    in extendParagraphBlock (created by Filters(BlockEdit))
    in Filters(BlockEdit) (created by BlockListBlock)
    in BlockCrashBoundary (created by BlockListBlock)
    in div (created by BlockListBlock)
    in div (created by BlockListBlock)
    in BlockListBlock (created by Filters(BlockListBlock))
    in Filters(BlockListBlock) (created by WrappedComponent)
    in WrappedComponent (created by Connect(WrappedComponent))
    in Connect(WrappedComponent) (created by BlockList)
    in div (created by BlockList)
    in BlockList (created by Connect(BlockList))
    in Connect(BlockList) (created by VisualEditor)
    in div (created by WritingFlow)
    in WritingFlow (created by Connect(WritingFlow))
    in Connect(WritingFlow) (created by VisualEditor)
    in div (created by VisualEditor)
    in VisualEditor (created by Connect(VisualEditor))
    in Connect(VisualEditor) (created by Layout)
    in div (created by Layout)
    in div (created by Layout)
    in div (created by Layout)
    in Layout (created by _class)
    in div (created by _class)
    in _class (created by Connect(_class))
    in Connect(_class)
    in ErrorBoundary
    in Provider (created by EditorProvider)
    in EditableProvider (created by EditorProvider)
    in SlotFillProvider (created by EditorProvider)
    in APIProvider (created by EditorProvider)
    in DropZoneProvider (created by EditorProvider)
    in EditorProvider

Any ideas of what might be wrong with my code?

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 5, 2018

Member
var element = el( BlockEdit, props );

Did you check if a key is passed as part of the prop object in this element?

Member

gziolo commented Jan 5, 2018

var element = el( BlockEdit, props );

Did you check if a key is passed as part of the prop object in this element?

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Jan 5, 2018

No, key is not passed as part of the prop object in this element. Is it something I should do, or should I report it as a bug?

kmgalanakis commented Jan 5, 2018

No, key is not passed as part of the prop object in this element. Is it something I should do, or should I report it as a bug?

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 5, 2018

Member

You should add it yourself to the object passed as props. React needs those keys when using an array as children. It’s very annoying 🙁

Member

gziolo commented Jan 5, 2018

You should add it yourself to the object passed as props. React needs those keys when using an array as children. It’s very annoying 🙁

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Jan 5, 2018

Unfortunately I'm not able to add it to the props object directly. Even though I do a props.key = my-key the key property is not added. I have to clone the props object and add the key property there. Do you have an idea why this happens?

kmgalanakis commented Jan 5, 2018

Unfortunately I'm not able to add it to the props object directly. Even though I do a props.key = my-key the key property is not added. I have to clone the props object and add the key property there. Do you have an idea why this happens?

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 5, 2018

Member

You should always treat props as an immutable object. It might be exposed as read-only by React. I’m not sure. Object.assign will do the trick for you:

Object.assign( { key: 'my-key' }, props );

Member

gziolo commented Jan 5, 2018

You should always treat props as an immutable object. It might be exposed as read-only by React. I’m not sure. Object.assign will do the trick for you:

Object.assign( { key: 'my-key' }, props );

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Jan 5, 2018

Good to know, thanks a lot for you help and your prompt answers.

kmgalanakis commented Jan 5, 2018

Good to know, thanks a lot for you help and your prompt answers.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 5, 2018

Member

My pleasure 😃Extend Gutenberg and prosper!

Member

gziolo commented Jan 5, 2018

My pleasure 😃Extend Gutenberg and prosper!

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 26, 2018

@kmgalanakis is your code working? Would you mind sharing the fixes. Tried it on my end and no luck. Thanks!

phpbits commented Jan 26, 2018

@kmgalanakis is your code working? Would you mind sharing the fixes. Tried it on my end and no luck. Thanks!

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Jan 26, 2018

@phpbits are you talking about the code that fixes the Warning: Each child in an array or iterator should have a unique "key" prop.?

Following Greg's suggestion I did something like

var clonedProps = Object.assign( {}, props );
clonedProps.key = 'test-key';
var element = el( BlockEdit, clonedProps );

Let me know if this fixes your issue.

kmgalanakis commented Jan 26, 2018

@phpbits are you talking about the code that fixes the Warning: Each child in an array or iterator should have a unique "key" prop.?

Following Greg's suggestion I did something like

var clonedProps = Object.assign( {}, props );
clonedProps.key = 'test-key';
var element = el( BlockEdit, clonedProps );

Let me know if this fixes your issue.

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 26, 2018

@kmgalanakis I didn't get the warning but the code doesn't do anything. Tried adding console.log too to check if it's working but it's not. Do you have the updated working one? Thanks!

phpbits commented Jan 26, 2018

@kmgalanakis I didn't get the warning but the code doesn't do anything. Tried adding console.log too to check if it's working but it's not. Do you have the updated working one? Thanks!

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 26, 2018

@kmgalanakis Sorry! Updated my gutenberg plugin and it's working fine now! Thanks!

phpbits commented Jan 26, 2018

@kmgalanakis Sorry! Updated my gutenberg plugin and it's working fine now! Thanks!

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Jan 26, 2018

@phpbits I'm glad to hear that! Thanks for letting me know.

kmgalanakis commented Jan 26, 2018

@phpbits I'm glad to hear that! Thanks for letting me know.

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 26, 2018

@kmgalanakis I'm trying to modify the code and add elements on the inspector control instead. Would you mind helping me? Thanks!

edited:
I added var InspectorControls = wp.blocks.InspectorControls; and use it instead of BlockControls and it's working :)

phpbits commented Jan 26, 2018

@kmgalanakis I'm trying to modify the code and add elements on the inspector control instead. Would you mind helping me? Thanks!

edited:
I added var InspectorControls = wp.blocks.InspectorControls; and use it instead of BlockControls and it's working :)

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 29, 2018

@kmgalanakis would you mind if I ask you another question? Do you know how to use classnames on this code : https://github.com/WordPress/gutenberg/blob/master/blocks/hooks/custom-class-name.js when it's written same as yours. It's on addSaveProps(). I'm trying to add custom class to each blocks too. Thanks!

phpbits commented Jan 29, 2018

@kmgalanakis would you mind if I ask you another question? Do you know how to use classnames on this code : https://github.com/WordPress/gutenberg/blob/master/blocks/hooks/custom-class-name.js when it's written same as yours. It's on addSaveProps(). I'm trying to add custom class to each blocks too. Thanks!

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Jan 29, 2018

If I get your question right, you need to do something like

className: classnames( 'components-toolbar', 'another-class', 'lorem' )

If this is not what you are looking for, please elaborate more.

kmgalanakis commented Jan 29, 2018

If I get your question right, you need to do something like

className: classnames( 'components-toolbar', 'another-class', 'lorem' )

If this is not what you are looking for, please elaborate more.

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 29, 2018

@kmgalanakis I'm creating a filter function similar to addSaveProps : https://github.com/WordPress/gutenberg/blob/master/blocks/hooks/custom-class-name.js . Where I'll be adding new classes on extraProps.className but this isn't working :

extraProps.className = extraProps.className + ' sample_class';

So classnames() probably should be use and I need help on using this function. Thanks!

phpbits commented Jan 29, 2018

@kmgalanakis I'm creating a filter function similar to addSaveProps : https://github.com/WordPress/gutenberg/blob/master/blocks/hooks/custom-class-name.js . Where I'll be adding new classes on extraProps.className but this isn't working :

extraProps.className = extraProps.className + ' sample_class';

So classnames() probably should be use and I need help on using this function. Thanks!

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 29, 2018

Just realize that it's same as your example. I need something like that :) Thanks @kmgalanakis !

phpbits commented Jan 29, 2018

Just realize that it's same as your example. I need something like that :) Thanks @kmgalanakis !

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Jan 29, 2018

Then, It should be:

extraProps.className = classnames( extraProps.className , 'sample_class' );

kmgalanakis commented Jan 29, 2018

Then, It should be:

extraProps.className = classnames( extraProps.className , 'sample_class' );
@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 29, 2018

@kmgalanakis That's the first I've tried but classnames is not defined. Tried wp.classnames wp.component.classnames wp.blocks.classnames and all not working. If I can't figure it out, I'll probably just add the value on Additional CSS Class field. Thanks!

phpbits commented Jan 29, 2018

@kmgalanakis That's the first I've tried but classnames is not defined. Tried wp.classnames wp.component.classnames wp.blocks.classnames and all not working. If I can't figure it out, I'll probably just add the value on Additional CSS Class field. Thanks!

@kmgalanakis

This comment has been minimized.

Show comment
Hide comment
@kmgalanakis

kmgalanakis Jan 29, 2018

On the root of your project, inside a terminal execute npm install --save classnames.

Then, on the top of your script, add import classnames from 'classnames';

You should be good to go.

kmgalanakis commented Jan 29, 2018

On the root of your project, inside a terminal execute npm install --save classnames.

Then, on the top of your script, add import classnames from 'classnames';

You should be good to go.

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 29, 2018

@kmgalanakis I'm using ES5 JavaScript and classnames importing is probably not an option. Thanks!

phpbits commented Jan 29, 2018

@kmgalanakis I'm using ES5 JavaScript and classnames importing is probably not an option. Thanks!

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 29, 2018

Member

Yes, you can simply put all the class names in array and join with a space instead 😃

Member

gziolo commented Jan 29, 2018

Yes, you can simply put all the class names in array and join with a space instead 😃

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Jan 29, 2018

Member

Or even concatenate those strings.

Member

gziolo commented Jan 29, 2018

Or even concatenate those strings.

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 29, 2018

@gziolo concatenation is giving Block validation: Block validation failed error.

phpbits commented Jan 29, 2018

@gziolo concatenation is giving Block validation: Block validation failed error.

@phpbits

This comment has been minimized.

Show comment
Hide comment
@phpbits

phpbits Jan 30, 2018

@gziolo and @kmgalanakis I just added the values on Additional CSS Classes field :) Thank you very much!

phpbits commented Jan 30, 2018

@gziolo and @kmgalanakis I just added the values on Additional CSS Classes field :) Thank you very much!

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