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.

Sign up for GitHub

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

Jump to bottom

fix issue 24539, show external icon on help button #24622

Merged
merged 3 commits into from
Sep 8, 2020
Merged

fix issue 24539, show external icon on help button #24622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5a9980
fix issue 24539, show external icon on help button
Aug 18, 2020
08763cd
add visually hidden text for external link item
Aug 20, 2020
dbef172
unite duplicate import
Sep 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/edit-post/src/plugins/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
/**
* WordPress dependencies
*/
import { MenuItem } from '@wordpress/components';
import { MenuItem, VisuallyHidden } from '@wordpress/components';
import { external } from '@wordpress/icons';
import { __ } from '@wordpress/i18n';
import { registerPlugin } from '@wordpress/plugins';
import { addQueryArgs } from '@wordpress/url';
Expand Down Expand Up @@ -38,13 +39,20 @@ registerPlugin( 'edit-post', {
<CopyContentMenuItem />
<MenuItem
role="menuitem"
icon={ external }
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks so much for picking this up 👍

In addition to the icon, the other thing that's usually present on these external links is some visually hidden text for screenreader users.

I think you should be able to follow how the External Link component does that for implementing it on these two MenuItems:
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/external-link/index.js#L38-L43

Alternatively, as @jasmussen proposes a prop for the MenuItem component could be an option so that it adds the icon and the visually hidden text implicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the hint, i didn't knew about that. I have added the visually hidden text like in the example, works well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@talldan I thought about the alternative idea from @jasmussen and prepared another PR #24713 with the enhancement of the MenuItem. Maybe you could check if this is a better solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll refresh my PRs and rebase them off whatever lands here. In the mean time, this PR should not be held up.

This is what the preview dropdown looks like:

Screenshot 2020-08-24 at 09 50 02

The same feedback applies to that, it looks a little weird on the left. But because this PR looks the same, I say let's move this forward. 👍 👍 from a design POV and thank you for the PR! If this gets a quick sanity check, we should land it.

href={ __(
'https://wordpress.org/support/article/wordpress-editor/'
) }
target="_blank"
rel="noopener noreferrer"
>
{ __( 'Help' ) }
<VisuallyHidden as="span">
{
/* translators: accessibility text */
__( '(opens in a new tab)' )
}
</VisuallyHidden>
</MenuItem>
</>
) }
Expand Down
11 changes: 9 additions & 2 deletions packages/edit-site/src/plugins/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,12 @@ import downloadjs from 'downloadjs';
/**
* WordPress dependencies
*/
import { MenuItem } from '@wordpress/components';
import { MenuItem, VisuallyHidden } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { registerPlugin } from '@wordpress/plugins';
import { addQueryArgs } from '@wordpress/url';
import apiFetch from '@wordpress/api-fetch';
import { download } from '@wordpress/icons';
import { download, external } from '@wordpress/icons';

/**
* Internal dependencies
Expand Down Expand Up @@ -56,13 +56,20 @@ registerPlugin( 'edit-site', {
</MenuItem>
<MenuItem
role="menuitem"
icon={ external }
href={ __(
'https://wordpress.org/support/article/wordpress-editor/'
) }
target="_blank"
rel="noopener noreferrer"
>
{ __( 'Help' ) }
<VisuallyHidden as="span">
{
/* translators: accessibility text */
__( '(opens in a new tab)' )
}
</VisuallyHidden>
</MenuItem>
</ToolsMoreMenuGroup>
</>
Expand Down