Add No Follow toggle to Link Control #48722

tomdevisser · 2023-03-03T11:32:48Z

What, why and how?

After almost three years and a lot of fruitful discussion, it's finally here. The ability to add a nofollow rel to a link.

Everything works in the backend as shown on the screengrab. The only thing I couldn't figure out in the time I had now was how to save this to the markup so it also renders correctly on the frontend. Hope someone can help with this or point me in the right direction.

Testing Instructions

Add a paragraph block
Insert a link
Check the rel attribute

A few important notes

My linter gave a lot of warnings on the useEffect dependency arrays. Someone should look into that, but probably for a separate issue. I didn't want to autocorrect them all, as that could mess up the tests.
I don't really like the fact the popup closes automatically after toggling. This was already the case with the new tab option, but especially when there's more options you don't want to open the toggle for every single setting. Also worth creating another issue for.
There's a few other refactors possible, like reusing the titles for the settings in the constants file, on the BottomSheet.SwitchCell components (link-settings-screen.native.js:213). These do however have lower priority and I will assign those to myself after this PR gets merged.

Don't see this as finished yet, but I think we now have a very good starting point to start adding this feature!

Fixes #23011

nofollow.mov

After almost three years and a lot of fruitful discussion, it's finally here. The ability to add a nofollow rel to a link. Everything works in the backend as shown on the screengrab. The only thing I couldn't figure out in the time I had was how to save this to the markup so it also renders correctly on the frontend. Hope someone can help with this or point me in the right direction. Fixes WordPress#23011

tomdevisser · 2023-03-03T11:43:51Z

One more thing: I've never written a test in a big project like this, so if someone can help me with updating any tests where necessary, that would also be greatly appreciated. 🙂

getdave · 2023-03-03T12:32:10Z

Thank you. This is on my radar to review. As explained we are in the WP 6.2 release cycle so I may not be able to get to this straight away.

Also tagging in @jameskoster as he's been working on the Design here.

tomdevisser · 2023-03-03T12:33:51Z

@getdave Good luck with the release!

getdave · 2023-03-03T12:34:20Z

A good place to add tests is in packages/block-editor/src/components/link-control/test/index.js. That's the base component.

That said I think the tests are suitale generic as to not need anything additional for this new control. You'll see there's already a test should display a setting control with correct default state for each of the custom settings provided.

The other option is an e2e test in packages/e2e-tests/specs/editor/various/links.test.js.

getdave · 2023-03-03T12:36:18Z

I don't really like the fact the popup closes automatically after toggling. This was already the case with the new tab option, but especially when there's more options you don't want to open the toggle for every single setting. Also worth creating another issue for.

Agreed. That's definitely one for another Issue. In fact I think we already have one 🤔

jameskoster · 2023-03-03T14:40:50Z

Nice, thanks for working on this :)

I don't really like the fact the popup closes automatically after toggling.

+1.

The label looks a little long but I appreciate the context. Maybe we can use some helper text instead?

getdave · 2023-03-07T13:44:46Z

I think the test failures here are legit. We'll need to look into those.

tomdevisser · 2023-03-08T12:08:00Z

I think the test failures here are legit. We'll need to look into those.

Do you know anyone that can help with this? :)

getdave · 2023-03-09T08:18:15Z

I am happy to try and support that. Likely post-WP 6.2 release.

getdave · 2023-03-15T09:37:22Z

I haven't forgotten about this. I've been super busy with 6.2. It's still on my list.

getdave · 2023-03-31T09:25:05Z

Let me take a look at this today.

getdave

Let's try and get a @WordPress/gutenberg-design review on this one.

We should also add some e2e test coverage. I'm going to take a look at how we can extend what we have.

Apologies for the delay here but it's been an extremely busy period with the 6.2 release.

getdave · 2023-03-31T09:31:00Z

packages/format-library/src/link/modal-screens/link-settings-screen.native.js

@@ -42,6 +42,9 @@ const LinkSettingsScreen = ( {
 	const [ opensInNewWindow, setOpensInNewWindows ] = useState(
 		activeAttributes.target === '_blank'
 	);
+	const [ markedAsNoFollow, setMarkedAsNoFollow ] = useState(
+		( activeAttributes.rel = 'nofollow' )


Suggested change

( activeAttributes.rel = 'nofollow' )

( activeAttributes.rel?.includes( 'nofollow' ) )

Are there other options for this we need to consider?

I'd say let's keep the PR's simple and small, we can always add more later but that prevents them from getting to complex or going stale

No sorry I was thinking if the attributes have other settings on rel. For example noopener like this PR accounts for elsewhere (e.g. packages/format-library/src/link/inline.js).

+1 to what @getdave asks as it is now it doesn't work because it expects a boolean value for markedAsNoFollow

jasmussen · 2023-03-31T09:42:14Z

Just following up on this comment:

We should show the help text below both control and toggle to match the other instances where this is used:

As for the PR itself, it seems clear enough that nofollow is a property you should be able to set on a link. It's also a somewhat advanced control, so to me this is an argument for keeping the "Advanced" toggle. This one:

getdave · 2023-03-31T10:06:24Z

packages/block-editor/src/components/link-control/use-internal-input-value.js

@@ -16,7 +16,7 @@ export default function useInternalInputValue( value ) {
 		if ( value && value !== internalInputValue ) {
 			setInternalInputValue( value );
 		}
-	}, [ value ] );
+	}, [ internalInputValue, value ] );


Suggested change

}, [ internalInputValue, value ] );

}, [ value ] );

Please can we revert this change and use the eslint syntax to disable the warning?

The original code is clearly faulty (that is my fault) but we should avoid overcomplicating this PR by adding the dep here.

I will raise a separate Issue to fix this problem.

Once we revert this change we might see tests passing again 🤞

getdave · 2023-03-31T10:15:07Z

packages/block-editor/src/components/link-control/constants.js

+		title: __(
+			'Search engines should ignore this link (mark as nofollow)'
+		),


Suggested change

title: __(

'Search engines should ignore this link (mark as nofollow)'

),

title: __(

'No follow'

),

help: __(

'Search engines should ignore this link'

),

Let's extend this and then make changes within settings.js to optionally include the help prop on the ToggleControl.

getdave · 2023-03-31T10:18:44Z

We also need to make the new setting optional in the UI and handling code.

For an example of what I mean try adding a link to a Navigation block and try toggling the No follow setting. It doesn't work because the Nav block doesn't handle it yet.

We can patch the Nav block code, but there will be other (3rd party) consumers of this code who may not know about this change and won't have time to adjust their code to account for it.

One idea was that we could filter out any settings that are no included in the value object. We can do this in settings.js:

// filter out any settings not in the value
const filteredSettings = settings.filter( ( setting ) =>
	Object.keys( value ).includes( setting.id )
);

We would need to add a test for this type of scenario in packages/block-editor/src/components/link-control/test/index.js.

The only thing I couldn't figure out in the time I had now was how to save this to the markup so it also renders correctly on the frontend. Hope someone can help with this or point me in the right direction.

Could you elaborate on this a little? I tried with a paragraph block and it rendered on the front of the site.

geriux · 2023-03-31T14:21:45Z

I've just assigned myself to check the native file changes. I'll report back once I've tested these new changes 👍

geriux · 2023-04-03T16:33:33Z

packages/format-library/src/link/modal-screens/link-settings-screen.native.js

+						<BottomSheet.SwitchCell
+							icon={ external }
+							label={ __(
+								'Search engines should ignore this link (mark as nofollow)'


This is a bit long for mobile so maybe we could use just the part "No follow" as suggested in this comment.

geriux · 2023-04-03T16:37:30Z

packages/format-library/src/link/modal-screens/link-settings-screen.native.js

+							label={ __(
+								'Search engines should ignore this link (mark as nofollow)'
+							) }
+							value={ markedAsNoFollow }


Since we are using the markedAsNoFollow value here and it's within a useMemo we should include it in the dependencies so it gets the updated value.

geriux · 2023-04-03T16:48:09Z

packages/format-library/src/link/modal-screens/link-settings-screen.native.js

@@ -99,6 +102,7 @@ const LinkSettingsScreen = ( {
 		const format = createLinkFormat( {
 			url,
 			opensInNewWindow,
+			markedAsNoFollow,


Suggested change

markedAsNoFollow,

markAsNoFollow: markedAsNoFollow,

Since the names are different we should update it so the right value gets to createLinkFormat

geriux · 2023-04-03T16:51:47Z

Hey @tomdevisser 👋 thank you for working on including these changes for the mobile editor as well, I've shared some comments/suggestions, let me know what you think. Thanks!

jonoalderson · 2023-04-11T07:42:19Z

Weighing in from an SEO perspective; a few things to watch out for:

It's nofollow, not no follow.
It doesn't mean that search engines won't follow a link; it's a hint which asks them not to pass value to the destination URL. Google et al will still follow/crawl links with a nofollow attribute for discovery purposes.
This will need to anticipate interacting gracefully with links with a sponsored rel attribute, in the same way that we handle in Yoast SEO.

Given the above, I'd be keen to make sure that this can be disabled, otherwise, it'll end up duplicating and degrading the functionality we offer in Yoast SEO.

getdave · 2023-04-18T08:50:01Z

@jonoalderson Thanks for this useful feedback.

Given the above, I'd be keen to make sure that this can be disabled, otherwise, it'll end up duplicating and degrading the functionality we offer in Yoast SEO.

I'm curious about this. My previous review suggested that by way of backwards compatibility, this PR must be updatedd to not include any setting which is not provided in the settings object.

Would this be sufficient to cater for 3rd party extenders (Yoast is one of these) who have an existing implementation which should be extending whatever is provided by Core Gutenberg.

It sounds like we are thinking along the same lines, but I want to check we're not making any exceptions for any specific extenders 🙇‍♂️

@tomdevisser No pressure at all, but do you anticipate being able to continue working on this PR?

tomdevisser · 2023-04-20T14:46:19Z

@getdave I'm afraid some other higher priority issues are taking up all of my time at the moment. If you'd want to, feel free to move this forward!

getdave · 2023-04-20T15:37:33Z

@getdave I'm afraid some other higher priority issues are taking up all of my time at the moment. If you'd want to, feel free to move this forward!

Same here. No rush 👍

getdave · 2023-08-31T12:20:48Z

A PR for this has been merged to Gutenberg trunk in #53945 this can be safely closed.

Thank you for the agency in pushing this forward 🙇

tomdevisser requested review from ellatrix, fluiddot and getdave as code owners March 3, 2023 11:32

tomdevisser mentioned this pull request Mar 3, 2023

Link Control: Add more controls. #23011

Open

tomdevisser self-assigned this Mar 3, 2023

tomdevisser added [Type] Enhancement A suggestion for improvement. [Block] Paragraph Affects the Paragraph Block [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) labels Mar 3, 2023

tomdevisser requested review from karmatosed, paaljoachim and richtabor March 3, 2023 11:36

getdave requested a review from jameskoster March 3, 2023 12:34

getdave requested a review from SaxonF March 9, 2023 08:17

tomjn approved these changes Mar 31, 2023

View reviewed changes

getdave added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Mar 31, 2023

getdave reviewed Mar 31, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

getdave reviewed Mar 31, 2023

View reviewed changes

getdave mentioned this pull request Mar 31, 2023

Fix attempting to sync state in controlled Link Control #49506

Closed

getdave reviewed Mar 31, 2023

View reviewed changes

geriux self-requested a review March 31, 2023 14:20

getdave changed the title ~~Link Control: Add more controls~~ Add No Follow toggle to Link Control Mar 31, 2023

geriux reviewed Apr 3, 2023

View reviewed changes

jordesign removed the [Block] Paragraph Affects the Paragraph Block label Aug 18, 2023

getdave mentioned this pull request Aug 25, 2023

Adds 'nofollow' setting to inline links (rich text only) #53945

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add No Follow toggle to Link Control #48722

Add No Follow toggle to Link Control #48722

tomdevisser commented Mar 3, 2023 •

edited

Loading

tomdevisser commented Mar 3, 2023

getdave commented Mar 3, 2023

tomdevisser commented Mar 3, 2023

getdave commented Mar 3, 2023

getdave commented Mar 3, 2023

jameskoster commented Mar 3, 2023

getdave commented Mar 7, 2023

tomdevisser commented Mar 8, 2023

getdave commented Mar 9, 2023

getdave commented Mar 15, 2023

getdave commented Mar 31, 2023

getdave left a comment

getdave Mar 31, 2023

tomdevisser Mar 31, 2023

getdave Mar 31, 2023 •

edited

Loading

geriux Apr 3, 2023

jasmussen commented Mar 31, 2023 •

edited

Loading

This comment was marked as outdated.

getdave Mar 31, 2023

getdave Mar 31, 2023

getdave Mar 31, 2023

getdave Mar 31, 2023

getdave commented Mar 31, 2023 •

edited

Loading

geriux commented Mar 31, 2023

geriux Apr 3, 2023

geriux Apr 3, 2023

geriux Apr 3, 2023

geriux commented Apr 3, 2023

jonoalderson commented Apr 11, 2023 •

edited

Loading

getdave commented Apr 18, 2023

tomdevisser commented Apr 20, 2023 •

edited

Loading

getdave commented Apr 20, 2023

getdave commented Aug 31, 2023

	( activeAttributes.rel = 'nofollow' )
	( activeAttributes.rel?.includes( 'nofollow' ) )

Add No Follow toggle to Link Control #48722

Are you sure you want to change the base?

Add No Follow toggle to Link Control #48722

Conversation

tomdevisser commented Mar 3, 2023 • edited Loading

What, why and how?

Testing Instructions

A few important notes

tomdevisser commented Mar 3, 2023

getdave commented Mar 3, 2023

tomdevisser commented Mar 3, 2023

getdave commented Mar 3, 2023

getdave commented Mar 3, 2023

jameskoster commented Mar 3, 2023

getdave commented Mar 7, 2023

tomdevisser commented Mar 8, 2023

getdave commented Mar 9, 2023

getdave commented Mar 15, 2023

getdave commented Mar 31, 2023

getdave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave Mar 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Mar 31, 2023 • edited Loading

This comment was marked as outdated.

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

getdave commented Mar 31, 2023 • edited Loading

geriux commented Mar 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geriux commented Apr 3, 2023

jonoalderson commented Apr 11, 2023 • edited Loading

getdave commented Apr 18, 2023

tomdevisser commented Apr 20, 2023 • edited Loading

getdave commented Apr 20, 2023

getdave commented Aug 31, 2023

tomdevisser commented Mar 3, 2023 •

edited

Loading

getdave Mar 31, 2023 •

edited

Loading

jasmussen commented Mar 31, 2023 •

edited

Loading

getdave commented Mar 31, 2023 •

edited

Loading

jonoalderson commented Apr 11, 2023 •

edited

Loading

tomdevisser commented Apr 20, 2023 •

edited

Loading