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

Allow links in Plugins group in the More Menu #12309

Merged
merged 7 commits into from
Dec 3, 2018
Merged

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Nov 26, 2018

Description

Fixes #11570.

As a first iteration, we can offer PluginMoreMenuItem which takes onClick callback and inserts menu items inside Plugins group. it would allow to handle Modal as well. It might require some code though.

It also includes an option to pass url which turns the menu item into a link.

How has this been tested?

Copy those snippets into JS console and make sure that there are new items added in More Menu. Make sure it produces a valid HTML which follows accessibility guidelines:

  • button for items with onClick
  • link for items with url
( function() {
	const { createElement } = wp.element;
	const { PluginMoreMenuItem } = wp.editPost;
	const { registerPlugin } = wp.plugins;

	registerPlugin( 'my-plugin-more-menu-button', {
		render() {
			return createElement(
				PluginMoreMenuItem,
				{
					icon: 'smiley',
					onClick: function() { alert( 'Smile :)' ); },
				},
				'Smile :)'
			);
		},
	} );
} )();
( function() {
	const { get } = lodash;
	const { createElement } = wp.element;
	const { PluginMoreMenuItem } = wp.editPost;
	const { addQueryArgs } = wp.url;
	const { registerPlugin } = wp.plugins;

	registerPlugin( 'classic-editor-add-submenu', {
		render() {
			const url = addQueryArgs( document.location.href, { 'classic-editor': null } );
			const linkText = get( window, [ 'classicEditorPluginL10n', 'linkText' ] ) || 'Switch to Classic Editor';

			return createElement(
				PluginMoreMenuItem,
				{
					icon: 'editor-kitchensink',
					href: url,
				},
				linkText
			);
		},
	} );
} )();

Screenshots

screen shot 2018-11-26 at 11 49 31

TODO

  • Docs for PluginMoreMenuItem (similar to what we have here)
  • New unit tests
  • New e2e tests (similar to what we have here) it's indirectly tested by the sidebar menu item which uses this new component behind the scenes

Checklist:

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

@gziolo gziolo requested a review from azaozz November 26, 2018 11:13
@gziolo gziolo added [Status] In Progress Tracking issues with work in progress [Feature] Extensibility The ability to extend blocks or the editing experience [Type] Enhancement A suggestion for improvement. Needs Accessibility Feedback Need input from accessibility labels Nov 26, 2018
@azaozz
Copy link
Contributor

azaozz commented Nov 26, 2018

Works great here. This way a plugin can add a (simple) link to the menu when needed, or specify a callback for the onClick. Properly adds either an <a> tag or a <button> depending on whether href was passed in the props. Also adds all needed a11y attributes by default.

This "opens up" the menu for plugins and makes it very easy to use.

Tested with different settings and in Chrome, Firefox and Edge on Win10. All works well.

azaozz added a commit to WordPress/classic-editor that referenced this pull request Nov 26, 2018
- Update the UI for user settings.
- Add some caching of the options.
- Add a plugin to add submenu item in the Block Editor. Requires WordPress/gutenberg#12309 to be merged.
- Some cleanup.
@mtias mtias added this to the 4.7 milestone Nov 28, 2018
@gziolo gziolo force-pushed the update/links-more-menu branch from 68491d0 to f568fba Compare November 28, 2018 13:37
@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Nov 28, 2018
@gziolo gziolo self-assigned this Nov 28, 2018
@gziolo
Copy link
Member Author

gziolo commented Nov 28, 2018

@youknowriad - I addressed your feedback to make PluginMoreMenuItem more flexible and use it inside PluginSidebarMoreMenuItem.

I also added docs and tests, PR should be ready to go.

@gziolo gziolo force-pushed the update/links-more-menu branch from 7019545 to d24cabf Compare November 30, 2018 09:22
@gziolo
Copy link
Member Author

gziolo commented Nov 30, 2018

@tofumatt or @afercia - could you confirm this component is accessible?

@tofumatt tofumatt self-requested a review November 30, 2018 10:19
@gziolo gziolo force-pushed the update/links-more-menu branch from d24cabf to 75c6e05 Compare November 30, 2018 12:59
@gziolo gziolo force-pushed the update/links-more-menu branch from 75c6e05 to b1069fb Compare November 30, 2018 14:02
Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me accessibility-wise! Just a little comment about docs, really 😄

@afercia
Copy link
Contributor

afercia commented Dec 1, 2018

Looks good to me. ARIA menu items can be used for links. References:

https://www.w3.org/TR/wai-aria-1.1/#menu
https://www.w3.org/TR/wai-aria-1.1/#menuitem
https://www.w3.org/TR/wai-aria-practices-1.1/#menu

Navigation Menubar Example:
https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/menubar-1.html

It would be really nice to not allow developers to use href="#" links. Is there any way to check the passed url value and throw an error if the value is #? Currently, it is possible to do something like:

( function() {
	const { createElement } = wp.element;
	const { PluginMoreMenuItem } = wp.editPost;
	const { registerPlugin } = wp.plugins;

	registerPlugin( 'classic-editor-add-submenu-hash', {
		render() {
			const url = "#";
			const linkText = "Hash link";

			return createElement(
				PluginMoreMenuItem,
				{
					icon: 'editor-kitchensink',
					href: url,
				},
				linkText
			);
		},
	} );
} )();

and this is bad for semantics and accessibility. A button should be used instead.

Also a value like #non-existing-id would be bad, but I'm not sure there's an easy way to prevent this case.

@gziolo
Copy link
Member Author

gziolo commented Dec 2, 2018

It would be really nice to not allow developers to use href="#" links. Is there any way to check the passed url value and throw an error if the value is #? Currently, it is possible to do something like:

It would be interesting to explore whether we could detect using a hash pointing to the same url and render a button instead of an anchor. We might want to use window.location.hash instead as one of the onClick events (we already close the menu when user clicks menu/item).

@gziolo gziolo removed the Needs Accessibility Feedback Need input from accessibility label Dec 3, 2018
@gziolo gziolo merged commit 92eab64 into master Dec 3, 2018
@gziolo gziolo deleted the update/links-more-menu branch December 3, 2018 09:41
goldentroll added a commit to goldentroll/classic-editor that referenced this pull request Mar 13, 2023
- Update the UI for user settings.
- Add some caching of the options.
- Add a plugin to add submenu item in the Block Editor. Requires WordPress/gutenberg#12309 to be merged.
- Some cleanup.
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 [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants