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

Provide warning if href passed to PluginMoreMenuItem is an incomplete… #13405

Conversation

MigdaliaBrito
Copy link

@MigdaliaBrito MigdaliaBrito commented Jan 21, 2019

… URL fragment identifier (#)

Description

Addresses #12535. Added a console warning if href value being passed to the PluginMoreMenuItem component is a # sign.

How has this been tested?

Paste the following in the console:

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

	registerPlugin( 'my-plugin-more-menu-button', {
		render() {
			return createElement(
				PluginMoreMenuItem,
				{
					href: '#',
				},
				'Test Link'
			);
		},
	} );
} )();

This should trigger the following console warning:
Links should trigger navigation. Replace href value for PluginMoreMenuItem component with a navigable URL.

The above code adds the following link item in the More Items menu
screen shot 2019-01-21 at 2 32 59 pm

Screenshots

screen shot 2019-01-21 at 2 41 58 pm

Types of changes

This is just introducing a console warning to encourage accessibility best practices when creating links. This affects the More Options menu.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@MigdaliaBrito MigdaliaBrito reopened this Jan 21, 2019
@gziolo gziolo added the [Type] Code Quality Issues or PRs that relate to code quality label Jan 22, 2019
@gziolo gziolo requested a review from youknowriad January 22, 2019 09:25
@youknowriad
Copy link
Contributor

youknowriad commented Jan 22, 2019

Thanks for the contribution @MigdaliaBrito

As I already said on the issue, if we start adding those kind of warnings, we won't stop as we need 10 warnings per component. I won't block the PR but I'm not convinced we should do it.

@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label Feb 7, 2019
@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

I'm also sceptical about introducing this type of checks as part of the production code. If we want to have this kind of logic, we might want to start with linting or something similar. In general, I don't feel like we can prevent all bad use cases and we should try to mitigate it by doing a proper education in documentation explaining how to use this component properly.

@aduth
Copy link
Member

aduth commented Apr 24, 2019

As of the merging of #14151, there is now some prior art for how we can discourage invalid component usage at build-time rather than at runtime (it is possible to use these lint rules outside Gutenberg). For that reason, I'd suggest we try to coalesce around this approach to enforcing best practices, and I think #12535 would be an appropriate application of this pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Decision Needs a decision to be actionable or relevant [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants