ExternalLink: Replace icon with unicode arrow #60255
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jameskoster
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Agreed. |
|
Unsure if it's the best way to do it, but I pushed a commit which applies the underline only to text inside |
tyxla
reviewed
Mar 28, 2024
| icon={ external } | ||
| className="components-external-link__icon" | ||
| /> | ||
| ↗ |
There was a problem hiding this comment.
I'd advise against spacing with because it makes it way more difficult to define equal spacing across all browsers and to precisely define the margin we want to have on the left side of the icon. In general, using content features for styling purposes is discouraged for the same reasons.
| icon={ external } | ||
| className="components-external-link__icon" | ||
| /> | ||
| ↗ |
There was a problem hiding this comment.
Any concerns with font browser support? Are we certain that the user won't use a font that doesn't support this unicode arrow icon?
There was a problem hiding this comment.
It's possible, but in such cases I believe a fallback font would be used to display the correct symbol, with a slightly different appearance, e.g.:
| <VisuallyHidden as="span"> | ||
| { | ||
| /* translators: accessibility text */ | ||
| __( '(opens in a new tab)' ) | ||
| } | ||
| </VisuallyHidden> | ||
| <StyledIcon | ||
| icon={ external } | ||
| className="components-external-link__icon" |
There was a problem hiding this comment.
What happens with plugins that were relying on the components-external-link__icon classname to add styles to the icon?
There was a problem hiding this comment.
I wrapped the symbol in a span with this class, so if any plugins are targeting it their styles will be applied.
| > | ||
| { children } | ||
| <span style={ { textDecoration: 'underline' } }>{ children }</span> |
There was a problem hiding this comment.
I'd prefer defining those styles with SCSS or Emotion rather than with CSS. Inlining all those styles could get messy.
| @@ -9,13 +9,11 @@ import type { ForwardedRef } from 'react'; | |||
| */ | |||
| import { __ } from '@wordpress/i18n'; | |||
| import { forwardRef } from '@wordpress/element'; | |||
| import { external } from '@wordpress/icons'; | |||
There was a problem hiding this comment.
I do favor the idea of not relying on icons when we don't need to. It sure helps keep things lighter.
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { VisuallyHidden } from '../visually-hidden'; | ||
| import { StyledIcon } from './styles/external-link-styles'; |
There was a problem hiding this comment.
I favor removal of Emotion when we can. 👍
We already had classnames used here, so perhaps we should have used SCSS anyway.
| > | ||
| { children } | ||
| <span style={ { textDecoration: 'underline' } }>{ children }</span> |
There was a problem hiding this comment.
Also, this will break a bunch of tests that expect the link text to be a direct child of the link.
There was a problem hiding this comment.
spans inside a is valid html. So this would just require the tests to be updated, right?
There was a problem hiding this comment.
Tests are failing due to a string mismatch at the accessibility tree level, so it should be fixed by #60255 (comment), I think. (It's good that they failed and flagged the issue 🙂)
There was a problem hiding this comment.
Yes, after addressing Lena's abovementioned comment, things should be good.
|
Size Change: -103 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
mirka
approved these changes
Mar 28, 2024
Comment on lines
70
to
76
| <VisuallyHidden as="span"> | ||
| { | ||
| /* translators: accessibility text */ | ||
| __( '(opens in a new tab)' ) | ||
| } | ||
| </VisuallyHidden> | ||
| <StyledIcon | ||
| icon={ external } | ||
| className="components-external-link__icon" | ||
| /> | ||
| <span className="components-external-link__icon">↗</span> |
There was a problem hiding this comment.
We don't want this arrow character to be read out literally in screen readers. How about we move the accessibility text to the aria-label?
diff --git a/packages/components/src/external-link/index.tsx b/packages/components/src/external-link/index.tsx
index 7974560d78..3f09cdd92b 100644
--- a/packages/components/src/external-link/index.tsx
+++ b/packages/components/src/external-link/index.tsx
@@ -13,7 +13,6 @@ import { forwardRef } from '@wordpress/element';
/**
* Internal dependencies
*/
-import { VisuallyHidden } from '../visually-hidden';
import type { ExternalLinkProps } from './types';
import type { WordPressComponentProps } from '../context';
@@ -67,13 +66,15 @@ function UnforwardedExternalLink(
<span className="components-external-link__contents">
{ children }
</span>
- <VisuallyHidden as="span">
- {
+ <span
+ className="components-external-link__icon"
+ aria-label={
/* translators: accessibility text */
__( '(opens in a new tab)' )
}
- </VisuallyHidden>
- <span className="components-external-link__icon">↗</span>
+ >
+ ↗
+ </span>
</a>
/* eslint-enable react/jsx-no-target-blank */
);
mirka
changed the title
tyxla
approved these changes
Mar 29, 2024
Comment on lines
70
to
76
| <VisuallyHidden as="span"> | ||
| { | ||
| /* translators: accessibility text */ | ||
| __( '(opens in a new tab)' ) | ||
| } | ||
| </VisuallyHidden> | ||
| <StyledIcon | ||
| icon={ external } | ||
| className="components-external-link__icon" | ||
| /> | ||
| <span className="components-external-link__icon">↗</span> |
| } | ||
|
|
||
| .components-external-link__icon { | ||
| margin-left: 0.5ch; |
There was a problem hiding this comment.
Isn't that still unpredictable and varying? I wonder if we'd instead prefer a more fixed value that doesn't change based on the font.
There was a problem hiding this comment.
Not a strong feeling, but I like that using ch means the spacing is relative to the font.
If we used a fixed space then the gap could end up feeling too large for condensed font variants, or too small for extended ones.
There was a problem hiding this comment.
Sounds good, as long as it's intentional 👍
| icon={ external } | ||
| className="components-external-link__icon" | ||
| /> | ||
| ↗ |
| > | ||
| { children } | ||
| <span style={ { textDecoration: 'underline' } }>{ children }</span> |
There was a problem hiding this comment.
Yes, after addressing Lena's abovementioned comment, things should be good.
|
Nice. If we do this the icon should always have the same font weight, though. I.e. never be bold, always normal. |
Co-authored-by: Lena Morita <lena@jaguchi.com>
|
Looks like the tests will need updating here, if someone could assist with that I'd appreciate it 🤞 |
7dfb1da should do it. It's a basic change that reflects the fact that we now have an extra wrapper element there. |
|
There was also an e2e failure, which 10ae57f should resolve. |
|
Thank you :) |
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
|
We've run into an issue on Link Control, where the arrow is not visible on an external link. It's kind of an odd case, but previously the SVG was hidden manually via CSS. But with this iteration, that's not possible, as the arrow also announces the link opening in a new tab. Thoughts on what we should do to resolve this? #60439 (review) |
|
Sorry @richtabor I missed that comment! So the arrow needs to be visually hidden, but still announced by screen readers? Could we use For something neater would it make sense to add a prop to |
|
I think the main point of If you're sure about the accessibility implications of not showing an arrow in a specific use case context (like if you're sure there are enough context clues that it would open in a new tab), it might be better to just roll your own |
Yea, I don't think a specific prop for this is necessary. I'll probably use clip to hide it—it's probably fine. Thanks for hashing this with me team. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Use a unicode arrow symbol rather than the
externalicon inExternalink.Why?
Before
After
If we agree this is a good idea there could be some further refinements to try. For instance the arrow might work better without the underline: