Add external link functionality to the Button component #6786
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,18 @@ | ||
| # Button | ||
|
|
||
| This component is used to implement buttons and links. | ||
| It's also used to implement links that open in a new browser's tab. | ||
|
|
||
| ## Props | ||
|
|
||
| The component accepts the following props. Any additional props will be passed to the rendered `<a />` or `<button />` element. The presence of a `href` prop determines whether an anchor element is rendered instead of a button. | ||
|
|
||
| * `isPrimary`: (bool) whether the button is styled as a primary button. | ||
| * `isLarge`: (bool) whether the button is styled as a large button. | ||
| * `isSmall`: (bool) whether the button is styled as a small button. | ||
| * `isToggled`: (bool) whether the button is styled with an inversion of colors. | ||
| * `isBusy`: (bool) whether the button is styled with a "busy" animation. | ||
| * `disabled`: (bool) whether the `<button />` element is disabled. | ||
| * `href`: (string) if this property is added, it will use an `a` rather than a `button` element. | ||
| * `rel`: (string) the `rel` attribute for the `<a />` element. | ||
| * `target`: (string) the `target` attribute for the `<a />` element. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,21 +2,25 @@ | |
| * External dependencies | ||
| */ | ||
| import classnames from 'classnames'; | ||
| import { compact, uniq, includes, omit } from 'lodash'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { createElement, forwardRef } from '@wordpress/element'; | ||
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import Dashicon from '../dashicon'; | ||
| import './style.scss'; | ||
|
|
||
| export function Button( props, ref ) { | ||
| const { | ||
| href, | ||
| target, | ||
| rel = '', | ||
| isPrimary, | ||
| isLarge, | ||
| isSmall, | ||
|
|
@@ -25,28 +29,60 @@ export function Button( props, ref ) { | |
| className, | ||
| disabled, | ||
| focus, | ||
| children, | ||
| ...additionalProps | ||
| } = props; | ||
|
|
||
| const tag = href !== undefined && ! disabled ? 'a' : 'button'; | ||
| const tagProps = tag === 'a' ? { href, target, rel } : { type: 'button', disabled }; | ||
| const { icon = true } = additionalProps; | ||
|
There was a problem hiding this comment. As I see it, There was a problem hiding this comment. We should document |
||
| const isExternalLink = href && ( target && ! includes( [ '_self', '_parent', '_top' ], target ) ); | ||
|
|
||
| const classes = classnames( 'components-button', className, { | ||
| button: ( isPrimary || isLarge || isSmall ), | ||
| 'button-primary': isPrimary, | ||
| 'button-large': isLarge, | ||
| 'button-small': isSmall, | ||
| 'is-toggled': isToggled, | ||
| 'is-busy': isBusy, | ||
| 'external-link': isExternalLink, | ||
|
There was a problem hiding this comment. Are we inheriting a core style here? If this is our own modifier class defined only within the |
||
| 'components-icon-button': isExternalLink && icon && ( isPrimary || isLarge || isSmall ), | ||
|
There was a problem hiding this comment. One component shouldn't inherit the styles of another. In this case, I wonder if Another idea is to bake in |
||
| } ); | ||
|
|
||
| const getRel = () => { | ||
|
There was a problem hiding this comment. While technically fine as-is, defining this as a function seems a bit excessive when we could just assign as let { rel } = this.props;
if ( isExternalLink && rel !== null ) {
rel = uniq( compact( [
...rel.split( ' ' ),
'external',
'noreferrer',
'noopener',
] ) ).join( ' ' );
}
// ...
return createElement( tag, {
// ...
rel,
} ); |
||
| // Allow to omit the `rel` attribute passing a `null` value. | ||
| return rel === null ? rel : uniq( compact( [ | ||
| ...rel.split( ' ' ), | ||
| 'external', | ||
| 'noreferrer', | ||
| 'noopener', | ||
| ] ) ).join( ' ' ); | ||
| }; | ||
|
|
||
| const { opensInNewTabText = __( | ||
|
There was a problem hiding this comment. Same note re: |
||
| /* translators: accessibility text */ | ||
| '(opens in a new tab)' | ||
| ) } = additionalProps; | ||
|
|
||
| const OpensInNewTabScreenReaderText = isExternalLink && ( | ||
| <span className="screen-reader-text"> | ||
| { | ||
| // We need a space to separate this from previous text. | ||
| ' ' + opensInNewTabText | ||
| } | ||
| </span> | ||
| ); | ||
|
|
||
| const ExternalIcon = icon && isExternalLink && <Dashicon icon="external" />; | ||
|
There was a problem hiding this comment. This will behave funny if you assign const ExternalIcon = 0 && true && <Dashicon icon="external" />;... will evaluate to: const ExternalIcon = 0;And React happily outputs a literal (Only Unless you know that all members of the condition are boolean values, I'd suggest a ternary instead, where the fallback value is one of the above unrendered (most likely |
||
|
|
||
| return createElement( tag, { | ||
| ...tagProps, | ||
| ...omit( additionalProps, [ 'icon', 'opensInNewTabText' ] ), | ||
|
There was a problem hiding this comment. Another advantage of destructuring |
||
| className: classes, | ||
| autoFocus: focus, | ||
| rel: isExternalLink && getRel(), | ||
| ref, | ||
| }, children, OpensInNewTabScreenReaderText, ExternalIcon ); | ||
| } | ||
|
|
||
| export default forwardRef( Button ); | ||
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.
relhas specific logic when assigned asnull, which is not (but should be) documented here.