-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Make "opens in a new tab" message a reusable component #6245
Conversation
See related Trac ticket for core: https://core.trac.wordpress.org/ticket/43803 |
Noting here some feedback from @aduth on Slack:
|
7db0237
to
0643d0b
Compare
I've tried to refactor this component to avoid to split the a11y message in a separate component. One of the issues is that some links that open a new browser's tab are not "external" at all, for example the link to the permalink settings or the Preview link: while other links are actually external links, for example "Learn more about manual excerpts". So if the same component needs to be used for different cases, there's the need for a bit more flexibility.
Re: the focus style for High Contrast Mode, no worries about the changes because core will soon provide its own style for the links and all the buttons that don't have a If this approach makes sense, I'd like to have some feedback before refining some custom text, for example the categories block and the recent posts block would need some customized text to inform users those links open in a new tab only in the edit post screen. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the approach.
Since we're saying that this component could be used for non-external links, should its name be updated to reflect that? Is it misleading to call it ExternalLink
when we're using it to point to other pages within the admin?
className="blocks-format-toolbar__link-value" | ||
href={ formats.link.value } | ||
target="_blank" | ||
icon={ null } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is documented as a boolean, but we're passing as null
here. Would false
be most appropriate?
components/external-link/index.js
Outdated
|
||
const { disabled, isPrimary, isLarge, isSmall } = additionalProps; | ||
const targetToUse = target || '_blank'; | ||
const isDisabled = disabled || ! href; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case for rendering this without href
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if no href props is passed, it's rendered as a disabled button
components/external-link/index.js
Outdated
} = this.props; | ||
|
||
const { disabled, isPrimary, isLarge, isSmall } = additionalProps; | ||
const targetToUse = target || '_blank'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be assigned in destructure as the default value?
const {
// ...
target = '_blank',
// ...
} = this.props;
getRel() { | ||
const { rel = '' } = this.props; | ||
|
||
// Allow to omit the `rel` attribute passing a `null` value. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the rel prop default value is still external noreferrer noopener but it wouldn't be appropriate for internal links so it can be omitted passing a null value
What is the consequence of applying them anyways? Purely in the interest of a simpler implementation.
Edit: Alternatively, is there any way we could programmatically determine whether we'd want to omit the rel
? What's the criteria ? That it's within the same hostname ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I thought about that and also looked if there was some url
utility to determine when the link is internal or external. I'm afraid that would get a bit complex, there are different cases to cover e.g.:
options-permalink.php
/something
http://site-domain/something
etc.
On the other hand, is the rel value external
really necessary? I wouldn't mind to simplify, remove it and keep noreferrer noopener
also for internal links. Thoughts?
Yes I'd be all for renaming this component, as On the other hand, while working on this, I've realised also the |
P.S. also the branch and this PR should be renamed 😬 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yes, I think we have some challenges with how we've approached buttons, largely around accommodating two valid†-but-secondary uses:
- Navigable links which have the appearance of a button
- Semantically an
<a>
, with appearance of<button>
- Semantically an
- Text which appears as a link but does not navigate
- Semantically a
<button>
, with appearance of<a>
- Semantically a
We've awkwardly tried to bundle all of these behaviors into a single <Button>
component. An argument could be made for splitting out a separate <Link>
component, but it's not obvious how to achieve the above edge-cases without significant overlap between the implementations.
Reflecting on the changes here again, my hunch is to push forward with the single <Button>
component and build-in logic of the ExternalLink
when passed both a href
and target="_blank"
prop. This avoids the need to invent a name for the component and for developers to need to be concerned with its use.
If we do want to keep as implemented, maybe NewWindowLink
?
† There can be a debate on whether these are truly valid
components/external-link/style.scss
Outdated
} | ||
|
||
.wp-core-ui .editor-block-list__layout & { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Components should be generic and not assume anything of their surrounding context. If styles are needed, it should be applied as a descendent selector from block-list/style.scss
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build-in logic of the ExternalLink when passed both a href and target="_blank" prop.
OK. Worth noting the target value can also be a named target, as Gutenberg does for the Preview button e.g. target="wp-preview-7169"
532e7ed
to
ae2d91d
Compare
Sounds like the mere presence of |
Not sure, what if devs use |
As agreed on Slack I'm going to close this PR and start working on a new one to build-in the external link behavior in the |
Hi All. Are you sure you don't want to just add a little "open in new tab" option to buttons? #6786 (merging links and buttons) doesn't seem like it's going to happen any time soon. |
This PR splits out the
(opens in a new tab)
message from theExternalLink
component and makes it a reusable component. This way, it can be re-used where necessary, for example in the "Preview" link, etc.introduces aOpensInNewTabMessage
component(opens in a new window)
to(opens in a new tab)
: today it's more likely browsers will open new tabs instead of new windows; for consistency, I'll open a Trac ticket to update the message in coreuses the new component where necessaryNote:
Safari + VoiceOver incorrectly read out the message before the link, even if the message is placed after in the DOM; this is a known Safari + VoiceOver bug I think we shouldn't try to solve here and it's already tracked in core, see https://core.trac.wordpress.org/ticket/42006
See also
https://github.com/h5bp/html5-boilerplate/issues/1985
Problem, considering:
the current message could be confusing and could let users understand the link opens always in a new tab, also in the frontend. Should we differentiate the message in these cases? We could pass a
message
prop to the component to allow for custom messages.Any feedback very welcome.
Fixes #1105