WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.52k stars 4.21k forks source link

Allow CustomSelectControl component to be used to extend a block toolbar #34845

Open brezocordero opened 3 years ago

brezocordero commented 3 years ago

What problem does this address?

In the MailPoet plugin we extend the block toolbar of the RichText component using the CustomSelectControl component. Because the button doesn’t have the property 'data-toolbar-item': true, it doesn’t pass the toolbar check and we get the warning: Using custom components as toolbar controls is deprecated. Please use ToolbarItem or ToolbarButton components instead.

We have looked into using CustomSelectControl together with ToolbarItem but because of the missing property in the button, the warning remains.

What is your proposed solution?

To have the property 'data-toolbar-item' added to the button inside of CustomSelectControl so that it can pass the toolbar check and be used to extend a block toolbar without a warning.

HILAYTRIVEDI commented 3 years ago

I added the option in the CustomSelectControl . How do I check it?

mirka commented 2 years ago

We have looked into using CustomSelectControl together with ToolbarItem but because of the missing property in the button, the warning remains.

I'm wondering if this is a bug. What was the actual code that failed? Looking at the docs, I would assume that using the ToolbarItem wrapper would work for most (if not all) custom components.

mirka commented 2 years ago

I stumbled upon a related issue #32336. If you have a code snippet to reproduce, you could post it on that issue so we can consolidate and close this one.

Mamaduka commented 2 years ago

Hi, @brezocordero

This issue was reviewed in today's Editor Bug Scrub. As @mirka mentioned, it would be really helpful if you could provide a code snippet to reproduce the warning.

brezocordero commented 2 years ago

Thanks for the mention and to everyone that has looked into this. 🙌

In our plugin we are including the CustomSelectControl without a ToolBarItem inside of a BlockFormatControls it does work but we would like to follow the recommendations and remove the warning. This is our current code, the component inside of BlockFormatControls returns a CustomSelectControl.

When trying to add the ToolBarItem like the following (sorry it is not a snippet you can test, please let me know if you need something else):

    <ToolbarGroup>
      <ToolbarItem>
        {(toolbarItemHTMLProps) => (
          <CustomSelectControl
            {...toolbarItemHTMLProps}
            options={options}
            onChange={(selected): void => {
              const selectedItem = selected.selectedItem as Option;
              if (selectedItem.selectable) {
                onChange(selectedItem.value);
              }
            }}
            value={selectedValue}
            label={name}
            className="mailpoet-font-family-select"
            hideLabelFromVision={hideLabelFromVision}
          />
        )}
      </ToolbarItem>
    </ToolbarGroup>

I got the error Function components cannot be given refs. I thought I could pass the toolbarItemHTMLProps to the CustomSelectControl component, and that the button inside would get the ref and 'data-toolbar-item': true, but that didn't work.

Thanks for linking the similar issue on https://github.com/WordPress/gutenberg/issues/32336 , maybe my request to add 'data-toolbar-item': true to the button is not the right option.

In that case, please feel free to close the request, sorry for the noise and I would appreciate some guidance or documentation on how to use the CustomSelectControl component inside of a ToolBarItem.

mirka commented 2 years ago

Ok, thanks for the additional information!

Here is a snippet of two ways I can make it work:

const Test = () => {
    const options = [ { key: 'small', name: 'Small' } ];

    return (
        <ToolbarGroup>
            <ToolbarItem>
                { ( toolbarItemHTMLProps, ref ) => (
                    <CustomSelectControl
                        { ...toolbarItemHTMLProps }
                        ref={ ref }
                        options={ options }
                    />
                ) }
            </ToolbarItem>
            <ToolbarItem as={ CustomSelectControl } options={ options } />
        </ToolbarGroup>
    );
};

Let me know if either of those still don't work.

The documentation is definitely outdated in the render function case, so I'll get that fixed up along with #32336.

brezocordero commented 2 years ago

Let me know if either of those still don't work.

Hi @mirka

Thank you for the snippets.

It works but the deprecation warning Using custom components as toolbar controls is deprecated. Please use ToolbarItem or ToolbarButton components instead. is still there.

I have also tested the snippet outside our plugin, to rule out some conflict. I still got the deprecation warning. This is a snippet of code I used for testing:

const options = [
    {
        key: 'small',
        name: 'Small',
        style: { fontSize: '50%' },
    },
    {
        key: 'normal',
        name: 'Normal',
        style: { fontSize: '100%' },
    },
    {
        key: 'large',
        name: 'Large',
        style: { fontSize: '200%' },
    },
    {
        key: 'huge',
        name: 'Huge',
        style: { fontSize: '300%' },
    },
];

const MyCustomSelect = ( { onChange, value, activeAttributes } ) => {
    return (
        <BlockControls>
            <ToolbarGroup>
                <ToolbarItem>
                    { ( toolbarItemHTMLProps, ref ) => (
                        <CustomSelectControl
                            { ...toolbarItemHTMLProps }
                            ref={ ref }
                            label={ __( 'Font Size' ) }
                            hideLabelFromVision={ true }
                            options={ options }
                            value={
                                activeAttributes.fontSize
                                    ? options.find(
                                            ( option ) =>
                                                option.key ===
                                                activeAttributes.fontSize
                                      )
                                    : options[ 0 ]
                            }
                            onChange={ ( { selectedItem } ) => {
                                onChange(
                                    applyFormat( value, {
                                        type: 'my-custom-format/sample-select',
                                        attributes: {
                                            style: `font-size: ${ selectedItem.style.fontSize }`,
                                            fontSize: selectedItem.key,
                                        },
                                    } )
                                );
                            } }
                        />
                    ) }
                </ToolbarItem>
            </ToolbarGroup>
        </BlockControls>
    );
};

registerFormatType( 'my-custom-format/sample-select', {
    title: __( 'Sample select' ),
    tagName: 'span',
    className: 'sample-has-font-size',
    attributes: {
        style: 'style',
        fontSize: 'data-font-size',
    },
    edit: MyCustomSelect,
} );

Did you get the warning on your end?

Thanks again for your help!

mirka commented 2 years ago

Ah ok, I was testing the Toolbar component in isolation, but the warning does appear as you say when nesting inside BlockControls. (Along with some other ref passing warnings from Ariakit.)

To add some context (since I realize this isn't properly documented yet), I believe all this ToolbarItem abstraction is for accessibility, for example ensuring that arrow key navigation in the toolbar works consistently in the presence of custom toolbar items:

https://user-images.githubusercontent.com/555336/195144051-b3b08700-5e29-45e9-934b-d8bdaeac6d68.mp4

So whatever solution (temporary hack or otherwise) must be careful not to break this keyboard navigation part. In this sense, at least in most of my test cases, the warnings are warranted as it does in fact break keyboard navigation.

I think this requires some further investigation to address, but in the meantime I'll suggest using DropdownMenu instead of CustomSelectControl like in the documentation, since it is currently working. Or better yet, I guess ToolbarDropdownMenu.

Sorry for the back and forth, I'm not fully familiar with this component yet!

ryanwelcher commented 1 year ago

Reviewed in today's . We'll look at updating the code examples based on this comment