Skip to content
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

PluginMoreMenuItem implementation does not render MenuItem as expected #39474

Closed
AnthonyLedesma opened this issue Mar 15, 2022 · 2 comments · Fixed by #39517
Closed

PluginMoreMenuItem implementation does not render MenuItem as expected #39474

AnthonyLedesma opened this issue Mar 15, 2022 · 2 comments · Fixed by #39517
Assignees
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience

Comments

@AnthonyLedesma
Copy link
Contributor

AnthonyLedesma commented Mar 15, 2022

Description

Using the following implementation - I expect the plugin menu item to be rendered as a MenuItem as indicated in the docs for the props.other declaration. Unfortunately, this approach is rendering only a basic Button dom element which seems to be the default for the ActionItem component.

import { PluginMoreMenuItem } from '@wordpress/edit-post';

...
	return (
		<>
			<PluginMoreMenuItem onClick={ openModal }>
				{ __( 'Editor settings', 'coblocks' ) }
			</PluginMoreMenuItem>
			...
		</>
		)

I've found an alternate route that allows us to insert MenuItem components using the following:

/**
 * WordPress dependencies
 */
import { ActionItem } from '@wordpress/interface';
import { compose } from '@wordpress/compose';
import { MenuItem } from '@wordpress/components';
import { withPluginContext } from '@wordpress/plugins';

export default compose(
	withPluginContext( ( context, ownProps ) => {
		return {
			as: MenuItem,
			icon: ownProps.icon || context.icon,
			name: 'core/edit-post/plugin-more-menu',
		};
	} )
)( ActionItem );

By being able to use the MenuItem we gain the benefits of existing work done to handle aria attributes for example among other benefits such as styling consistency.

Here are a few helpful links to the related codebase.

return {
icon: ownProps.icon || context.icon,
name: 'core/edit-post/plugin-more-menu',
};

const PluginsMenuItem = ( props ) => (
// Menu item is marked with unstable prop for backward compatibility.
// They are removed so they don't leak to DOM elements.
// @see https://github.com/WordPress/gutenberg/issues/14457
<MenuItem
{ ...omit( props, [
'__unstableExplicitMenuItem',
'__unstableTarget',
] ) }
/>
);
(Show MenuItem implementation)

Step-by-step reproduction instructions

This is a code related bug and is reproducible with the following example code

import { PluginMoreMenuItem } from '@wordpress/edit-post';

...
	return (
		<>
			<PluginMoreMenuItem onClick={ openModal }>
				{ __( 'Editor settings', 'coblocks' ) }
			</PluginMoreMenuItem>
			...
		</>
		)

Screenshots, screen recording, code snippet

PluginMoreMenuItemBug

Environment info

WP 5.9.2
GB N/A
CoBlocks 2.22.4

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@AnthonyLedesma AnthonyLedesma changed the title Bug with PluginMoreMenuItem implementation PluginMoreMenuItem implementation does not render MenuItem as expected Mar 15, 2022
@Mamaduka Mamaduka added the [Feature] Extensibility The ability to extend blocks or the editing experience label Mar 16, 2022
@talldan
Copy link
Contributor

talldan commented Mar 16, 2022

@AnthonyLedesma That's a good point. Given the component is named MenuItem it's probably a bit deceiving that it's not rendering a menu item.

As a temporary workaround are you able to do something like this in your plugin?

import { MenuItem } from '@wordpress/components';
import { PluginMoreMenuItem } from '@wordpress/edit-post';

...
	return (
		<>
			<PluginMoreMenuItem as={ MenuItem } onClick={ openModal }>
				{ __( 'Editor settings', 'coblocks' ) }
			</PluginMoreMenuItem>
			...
		</>
        )

Your proposed fix makes sense to me, is that something you'd be willing to contribute in a PR? The only change I'd make is to have MenuItem as the default value, but still allow as to be overridden by the implementor. For example:

export default compose(
	withPluginContext( ( context, ownProps ) => {
		return {
			as: ownProps.as ?? MenuItem,
			icon: ownProps.icon || context.icon,
			name: 'core/edit-post/plugin-more-menu',
		};
	} )
)( ActionItem );

@AnthonyLedesma
Copy link
Contributor Author

Good call @talldan. It looks like passing in as={ MenuItem } as you have proposed in your example code does the job of rendering a MenuItem as expected.

I can work on a PR to make that MenuItem a default unless overridden.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Mar 16, 2022
@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants