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.

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

Link settings extensibility #13190

Closed
wants to merge 14 commits into from
Closed

Conversation

abotteram
Copy link
Contributor

@abotteram abotteram commented Jan 4, 2019

Description

Adds the possibility to expand the inline link settings via Slot/Fill and alter the link's attributes.

To achieve this I had to refactor the logic that determined the link's attributes because the only possibility was to change the link to an external link.

While working on this I found out that the aria-label logic is broken. The label will always be (opens in new window).

How has this been tested?

Add the NoFollowToggleWrapper somewhere in your code and make sure the component is always rendered. I simply copied it into packages/format-library/src/inline.js.

class NoFollowToggle extends Component {
	isChecked() {
		const { rel } = this.props.attributes;

		if ( ! rel ) {
			return false;
		}

		return rel.split( ' ' ).includes( 'nofollow' );
	}

	toggle() {
		const { setLinkAttributes, attributes } = this.props;

		const rel = attributes.rel;

		if ( this.isChecked() ) {
			if ( ! rel ) {
				return;
			}

			const newRel = rel.split( ' ' ).filter( ( relItem ) => {
				return relItem !== 'nofollow';
			} ).join( ' ' );

			setLinkAttributes( {
				...attributes,
				rel: newRel,
			} );
			return;
		}

		if ( ! rel ) {
			setLinkAttributes( {
				...attributes,
				rel: 'nofollow',
			} );
		} else {
			setLinkAttributes( {
				...attributes,
				rel: [ rel, 'nofollow' ].join( ' ' ),
			} );
		}
	}

	render() {
		return (
			<ToggleControl
				label={ 'No Follow' }
				checked={ this.isChecked() }
				onChange={ this.toggle.bind( this ) }
			/>
		);
	}
}

function NoFollowToggleWrapper() {
	return (
		<Fill name="LinkSettings">
			{ ( props ) => {
				return <NoFollowToggle { ...props } />;
			} }
		</Fill>
	);
}

Screenshots

screen shot 2019-01-04 at 15 47 14

Types of changes

New feature.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@abotteram abotteram added [Feature] Extensibility The ability to extend blocks or the editing experience [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Format library /packages/format-library labels Jan 4, 2019
@abotteram abotteram force-pushed the add/link-settings-extensibility branch 2 times, most recently from 7174da7 to dfaa108 Compare January 7, 2019 13:10
@IreneStr IreneStr requested a review from a team January 7, 2019 13:38
@atimmer atimmer requested a review from ellatrix January 7, 2019 16:24
@Chrico
Copy link
Contributor

Chrico commented Jan 9, 2019

Howdy,

any news here about the state or if something is missing? Is there already a scheudule when this will be included into Gutenberg? Currently there are a lot of options which could be possible be added to the LinkSettings to support e.G. custom data-attributes for tracking or even kind of theming to open modals.

@Soean
Copy link
Member

Soean commented Jan 9, 2019

@xyfi Can you add the No Follow example to the docs?

@youknowriad youknowriad added this to the 5.0 (Gutenberg) milestone Jan 9, 2019
@designsimply
Copy link
Member

Noting a related discussion from the #core-editor meeting in WP Slack today: https://wordpress.slack.com/archives/C02QB2JS7/p1547046547434300

@abotteram abotteram force-pushed the add/link-settings-extensibility branch 2 times, most recently from 1e10b34 to 8273c4c Compare January 16, 2019 12:14
@IreneStr IreneStr requested review from ellatrix and a team January 17, 2019 13:36
@mcsf
Copy link
Contributor

mcsf commented Jan 22, 2019

While working on this I found out that the aria-label logic is broken. The label will always be (opens in new window).

See #12325

@ellatrix ellatrix removed the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Jan 29, 2019
@GlennMartin1
Copy link

+1 to have this feature ASAP.

Any ETA? 5.1?

@mapk
Copy link
Contributor

mapk commented Feb 5, 2019

I'd like to point to #10128 in an effort to keep the styling and interaction consistent. This is what that interaction looks like.

link-button

@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

I'd like to point to #10128 in an effort to keep the styling and interaction consistent. This is what that interaction looks like.

This makes me wonder whether we should include more options it in the core by default without adding the overhead of slots and fills. Maybe we should add more options under 3 dots menu based on this PR and Button block. In particular, in the case of the Button block, these Link Settings sections feel super disconnected from the UI. We should think about the flow when someone has to use the keyboard to set link rel after providing url. I think it would be super annoying to tab as much to edit related settings.

@gziolo gziolo added the Needs Decision Needs a decision to be actionable or relevant label Feb 7, 2019
@gziolo gziolo removed this from the 5.2 (Gutenberg) milestone Mar 4, 2019
@abotteram abotteram force-pushed the add/link-settings-extensibility branch from 32f4218 to 0629911 Compare July 16, 2019 07:13
@abotteram abotteram mentioned this pull request Jul 16, 2019
5 tasks
@chriscct7
Copy link
Member

chriscct7 commented Aug 20, 2019

This makes me wonder whether we should include more options it in the core by default without adding the overhead of slots and fills.

There are a lot of options that could/should be there. Some of those are documented on #11599

For example:

  • nofollow links
  • open in new tab option
  • download attribute (for download links)
  • ability for plugins to add additional items as relevant (for example if you had a plugin that let you add tags like rel="friend" and things like that

I agree that core should likely add the first 3 as defaults, but a slot definitely seems like the way to solve this for the long term, and notably the lack of extensibility in this space is a huge regression from classic editor to Gutenberg

@Chrico
Copy link
Contributor

Chrico commented Aug 21, 2019

For me this is the wrong point to add this specific feature. It should be abstracted into a "general thing", which is re-used accross all Components which can have HTML-attributes [1].

This gernalized implementation should allow to add/remove key/value pairs (value could be maybe even a "selection of predefined allowed values"). Also blacklisting "core"-attributes (not changeable) - like on the Link-Component the href - should be possible.


[1] https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes

@paaljoachim paaljoachim mentioned this pull request Aug 26, 2019
4 tasks
@TwisterMc
Copy link

FYI: There's a plugin called EditorsKit that you can install and get nofollow functionality today. Not sure how they're doing it, but you can try it today.

@mcsf
Copy link
Contributor

mcsf commented Dec 16, 2019

The Link UI has evolved a lot since this PR was in active development. What is the status of the PR?

@draganescu
Copy link
Contributor

@xyfi considering that now the linking mechanism evolved into using LinkControl I will close this PR, since given the comments there is not a definitive path ahead and also even if it were the component to update is a completely new one. Thanks for the work it still remains valuable. I would consider opening an issue about how this extensibility should work? ( cc @Chrico )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience Needs Decision Needs a decision to be actionable or relevant [Package] Format library /packages/format-library
Projects
None yet
Development

Successfully merging this pull request may close these issues.