Add external link functionality to the Button component #6786

afercia · 2018-05-16T17:09:22Z

Replaces #6245

As agreed, this PR tries to bring in the functionalities formerly provided by the ExternalLink component into the Button component.

Before going ahead with polishing, some minor styling issues, documentation, and maybe tests, I'd greatly appreciate some feedback if this is really the desired behavior.

Worth reminding the Button component. already renders a button or a link, depending on the passed props. Now, when it has a href and target (not '_self', '_parent', '_top') props, it's considered an "external" link.

Fixes #1105

aduth · 2018-05-23T19:25:29Z

components/button/index.js

+		</span>
+	);
+
+	const ExternalIcon = icon && isExternalLink && <Dashicon icon="external" />;


This will behave funny if you assign icon={ 0 }, since the behavior of JS in an && assignment is to defer to the left value if it is falsey, i.e.

const ExternalIcon = 0 && true && <Dashicon icon="external" />;

... will evaluate to:

const ExternalIcon = 0;

And React happily outputs a literal 0 text (Example)

(Only false, "", null, or an undefined child will not be rendered)

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 null).

aduth · 2018-05-23T19:26:49Z

components/button/index.js

 		...additionalProps
 	} = props;

+	const tag = href !== undefined && ! disabled ? 'a' : 'button';
+	const tagProps = tag === 'a' ? { href, target, rel } : { type: 'button', disabled };
+	const { icon = true } = additionalProps;


As I see it, additionalProps are those which are not explicitly handled by the component itself. In this case, we are handling icon, and it should be included in the destructuring which occurs at the top of this function.

We should document icon. In particular, it's unclear to me whether this is always a boolean (where I might expect a prefix to indicate this such as hasIcon, showIcon) or an overloaded value where a boolean overrides a default.

aduth · 2018-05-23T19:27:02Z

components/button/index.js

+		] ) ).join( ' ' );
+	};
+
+	const { opensInNewTabText = __(


Same note re: additionalProps and assigning in initial destructuring.

aduth · 2018-05-23T19:30:01Z

components/button/index.js

 	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,


Are we inheriting a core style here? If this is our own modifier class defined only within the Button component, it should be named as such with an appropriate prefix, e.g. is-external-link (relevant guideline).

aduth · 2018-05-23T19:32:07Z

components/button/index.js

 	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,
+		'components-icon-button': isExternalLink && icon && ( isPrimary || isLarge || isSmall ),


One component shouldn't inherit the styles of another. In this case, I wonder if Button could render an itself as an IconButton when the condition applies (which would trickle down to rendering both the icon and another Button where maybe we omit the properties which would satisfy the condition to avoid infinite recursion).

Another idea is to bake in icon behavior into Button.

aduth · 2018-05-23T19:39:12Z

components/button/style.scss

+
+	&.external-link {
+		.dashicon {
+			width: 1.4em;


I see these existed previously, but shouldn't these styles be inherited from IconButton ? It's not obvious to me why we would we want the external link icon to be displayed any differently than any other icon button?

aduth · 2018-05-23T19:40:25Z

components/external-link/index.js

-import Dashicon from '../dashicon';
-import './style.scss';
-
-function ExternalLink( { href, children, className, rel = '', ...additionalProps } ) {


We should probably leave this for two versions per deprecation policy, with an added deprecated warning.

https://wordpress.org/gutenberg/handbook/reference/deprecated/
https://github.com/WordPress/gutenberg/blob/master/utils/deprecation.js

aduth · 2018-05-23T19:43:50Z

editor/components/block-list/style.scss

@@ -96,6 +96,11 @@
 		padding-left: 0;
 		padding-right: 0;
 	}
+
+	// Let buttons in the post content inherit the content font-size.
+	.wp-core-ui & .components-button.external-link {


Why do we need the .wp-core-ui context?

aduth · 2018-05-23T19:46:39Z

editor/components/post-permalink/index.js

@@ -112,6 +113,8 @@ class PostPermalink extends Component {
 						href={ getWPAdminURL( 'options-permalink.php' ) }
 						onClick={ this.addVisibilityCheck }
 						target="_blank"
+						icon={ false }
+						rel={ null }


So, as I recall, setting rel as null to override default behavior is needed because we don't want the external values to apply for links which open in a new tab but are within the admin? Can we infer this programmatically such that we don't impose this requirement on the developer to understand?

Example: https://github.com/aduth/dones/blob/0450c88429deb4546f72d20f915551ec62a33de3/src/components/link/index.js#L44-L47

aduth · 2018-05-23T19:47:33Z

components/button/README.md

 * `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.


rel has specific logic when assigned as null, which is not (but should be) documented here.

afercia · 2018-05-23T20:45:19Z

@aduth thanks for the detailed review 🙂 As I wrote in the initial comment, I was more expecting an initial feedback on the general approach 😉

Before going ahead with polishing, some minor styling issues, documentation, and maybe tests, I'd greatly appreciate some feedback if this is really the desired behavior.

Personally, I'm not so convinced that putting all these different behaviors into one single component is a good thing in the first place.

aduth · 2018-05-23T20:46:31Z

In that case: Sure, this looks perfectly reasonable to pursue. Can you elaborate on your hesitations?

afercia · 2018-05-23T21:13:14Z

Mostly ease of use by developers. It's a component named Button that actually renders either a button or a link or an external link with some logic (and assumptions) to render important attributes like rel, target, etc.

From an ease of use perspective, these are 3 very different behaviors that maybe should go in 3 separate components, regardless of how they look visually:

Button
Link
ExternalLink

There's also the IconButton component that doesn't help in making things clearer...

aduth · 2018-05-25T13:57:31Z

What is it about the abstraction of IconButton that makes it unclear that won't also be suffered by a separation of Button, Link, and ExternalLink components?

As noted previously in #6245 (review) , it's difficult to reconcile the various requirements here; things like navigable links which appear as buttons, or behavioral buttons which appear as navigable links. The hope with consolidating all behaviors into a single Button component is that if we can rely on a predictable set of assumptions, we can make it easier for the end-developer to pass certain attributes and it will be rendered "correct" on their behalf.

But this only works when we're consistent about how we're using the component. That's not the case currently (as a search for "<a " makes evident). Further, I worry it's not natural for a developer to intuitively consider a plain link as <Button href="/" isLink />. Between the inconsistent usage and unintuitiveness, it does appear the abstraction is failing us.

Would it be enough to separate this to two: Button and Link ?

A Button...

Is semantically: an element with Click/Enter/Space behavior (onClick)
Default appearance of button (see below open question in Link requiring href)
Handles icon behavior by assumptions based on incoming props (icon)
May be navigable (by presence of href), in which case it defers its rendering to an underlying Link

A Link...

Is semantically: a navigable element (href)
Default appearance of a link (<Button href="..."> may override)
Handles rel behavior by assumptions based on incoming props (href, target)
Open question: Must receive href prop?
- Could warn about incorrect usage otherwise, encouraging use of Button instead (avoid href="#" onClick="..."). But this requires that Button can assume the appearance of a link, which may be okay (as it does today with isLink prop).

A few notable advantages here:

Link is more intuitive to use in place of <a />, helping promote consistency
rel behavior is more natural to occur in Link (navigable), not Button
Eliminate IconButton as a separate component

afercia · 2018-05-25T16:04:21Z

Not arguing, but the original scope of this PR and the previous #6245 was to just make the usage of the accessible message (Opens in a new tab) easier, and now I'm scared 😱

Jokes apart 😉this:

Further, I worry it's not natural for a developer to intuitively consider a plain link as . Between the inconsistent usage and unintuitiveness, it does appear the abstraction is failing us.

Would it be enough to separate this to two: Button and Link ?

Makes a lot of sense to me. Personally, I'm used to think at meaning and semantics first, so to me a button is a button, a link is a link. Instead, naming a component based on its appearance doesn't seem so ideal to me.

I'm a bit concerned about the amount of work needed for such a big refactoring, and the potential temporary breakage it would introduce. I'm not sure I have the time to address all the necessary changes.

One more concern is about styling. Instead of deferring / overriding, I'd keep them well separated. However, this way the two components would share some styling:

a Button may look like a button
a Link may look like a button
_ (and vice-versa, with all the size variants, etc.)

This would lead to duplicating a lot of styles, with consequent maintainability issues. Wouldn't be possible to extend from a base styling (or a base component)?

Overall, I'd totally second your proposal as it would make developers life easier. Imagine a coding workflow... I need a link, well I just type a <Link ... component passing the props that are the most intuitive for a link. Yes, I should get warnings for incorrect usage. Same for buttons. I need a button? Yeah, let me just type <Button ... with some props. How they look should be a separate concern.

aduth · 2018-05-29T19:06:11Z

Not arguing, but the original scope of this PR and the previous #6245 was to just make the usage of the accessible message (Opens in a new tab) easier, and now I'm scared 😱

Sometimes it's the seemingly small changes that bring to light big underlying issues 😄

Makes a lot of sense to me. Personally, I'm used to think at meaning and semantics first, so to me a button is a button, a link is a link. Instead, naming a component based on its appearance doesn't seem so ideal to me.

Agreed 💯 .

This would lead to duplicating a lot of styles, with consequent maintainability issues. Wouldn't be possible to extend from a base styling (or a base component)?

I think this could make sense. How we deduplicate could have a few options:

Base styles defined in a common stylesheet, like what we have already in edit-popt/assets/stylesheets
- Should be stated that these are not intended to be specific to the post editor.
A "visually button" component, e.g.

const VisuallyButton = ( { className, ...props } ) => (
	<span className={ classnames( 'components-visually-button', className ) } { ...props } />
);

I'm a bit concerned about the amount of work needed for such a big refactoring, and the potential temporary breakage it would introduce. I'm not sure I have the time to address all the necessary changes.

If we can come to an agreement on the proposed implementation, would it be enough to create an issue with the proposed plan for another developer to pick up? I'm of course willing to help, though likewise with my current workload cannot commit to giving it the attention it deserves in a reasonable timeframe.

afercia · 2018-05-30T08:10:26Z

If we can come to an agreement on the proposed implementation,

FWIW seems to me the plan makes sense and would bring in clear advantages. I'd totally second it. I'm not the one who make decisions though. If the team agrees, I'd be happy to close this PR.

afercia · 2018-06-06T14:22:20Z

WCEU could be a good opportunity to have a look at this.

aduth · 2018-06-25T13:42:53Z

Issue at #7534

aduth · 2018-09-13T19:15:47Z

Closing as superseded by #7534, #9702

afercia added 2 commits May 16, 2018 19:02

First porting of functionalities from 6245 into the Button component.

fad53c3

Use on all linsk with target blank and remove ExternalLink.

2f8b3e2

afercia requested a review from aduth May 16, 2018 17:09

ZebulanStanphill mentioned this pull request May 19, 2018

No option to allow Button Block links to open in new Tab/Window #6843

Closed

aduth requested changes May 23, 2018

View reviewed changes

aduth mentioned this pull request Jun 25, 2018

Components: Separate Button / Link components, eliminate ExternalLink, IconButton #7534

Closed

afercia mentioned this pull request Jul 16, 2018

Add an icon and visually hidden a11y text to links that open in a new tab #7883

Merged

4 tasks

collinanderson mentioned this pull request Jul 31, 2018

Make "opens in a new tab" message a reusable component #6245

Closed

aduth closed this Sep 13, 2018

aduth deleted the add/button-external-link-behavior branch September 13, 2018 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add external link functionality to the Button component #6786

Add external link functionality to the Button component #6786

afercia commented May 16, 2018

aduth May 23, 2018

aduth May 23, 2018

aduth May 23, 2018

aduth May 23, 2018

aduth May 23, 2018

aduth May 23, 2018

aduth May 23, 2018

aduth May 23, 2018

aduth May 23, 2018

aduth May 23, 2018

aduth May 23, 2018 •

edited

Loading

afercia commented May 23, 2018

aduth commented May 23, 2018

afercia commented May 23, 2018 •

edited

Loading

aduth commented May 25, 2018

afercia commented May 25, 2018

aduth commented May 29, 2018

afercia commented May 30, 2018

afercia commented Jun 6, 2018

aduth commented Jun 25, 2018

aduth commented Sep 13, 2018

		* `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.

Add external link functionality to the Button component #6786

Add external link functionality to the Button component #6786

Conversation

afercia commented May 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth May 23, 2018 • edited Loading

Choose a reason for hiding this comment

afercia commented May 23, 2018

aduth commented May 23, 2018

afercia commented May 23, 2018 • edited Loading

aduth commented May 25, 2018

afercia commented May 25, 2018

aduth commented May 29, 2018

afercia commented May 30, 2018

afercia commented Jun 6, 2018

aduth commented Jun 25, 2018

aduth commented Sep 13, 2018

aduth May 23, 2018 •

edited

Loading

afercia commented May 23, 2018 •

edited

Loading