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.

Sign up for GitHub

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

Add No Follow toggle to Link Control #48722

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Add No Follow toggle to Link Control #48722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/block-editor/src/components/link-control/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,10 @@ export const DEFAULT_LINK_SETTINGS = [
id: 'opensInNewTab',
title: __( 'Open in new tab' ),
},
{
id: 'markAsNoFollow',
title: __(
'Search engines should ignore this link (mark as nofollow)'
),
Comment on lines +29 to +31
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
];
13 changes: 8 additions & 5 deletions packages/block-editor/src/components/link-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,14 @@ import { DEFAULT_LINK_SETTINGS } from './constants';
*
* @typedef WPLinkControlDefaultValue
*
* @property {string} url Link URL.
* @property {string=} title Link title.
* @property {boolean=} opensInNewTab Whether link should open in a new browser
* tab. This value is only assigned if not
* providing a custom `settings` prop.
* @property {string} url Link URL.
* @property {string=} title Link title.
* @property {boolean=} opensInNewTab Whether link should open in a new browser
* tab. This value is only assigned if not
* providing a custom `settings` prop.
* @property {boolean=} markAsNoFollow Whether link should be marked as
* `nofollow`. This value is only assigned
* if not providing a custom `settings` prop.
*/

/* eslint-disable jsdoc/valid-types */
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/link-control/use-internal-input-value.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export default function useInternalInputValue( value ) {
if ( value && value !== internalInputValue ) {
setInternalInputValue( value );
}
}, [ value ] );
}, [ internalInputValue, value ] );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#49506

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return [ internalInputValue, setInternalInputValue ];
}
2 changes: 2 additions & 0 deletions packages/format-library/src/link/inline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ function InlineLinkUI( {
type: activeAttributes.type,
id: activeAttributes.id,
opensInNewTab: activeAttributes.target === '_blank',
markAsNoFollow: activeAttributes.rel?.includes( 'nofollow' ),
title: richTextText,
...nextLinkValue,
};
Expand Down Expand Up @@ -118,6 +119,7 @@ function InlineLinkUI( {
? String( nextValue.id )
: undefined,
opensInNewWindow: nextValue.opensInNewTab,
markAsNoFollow: nextValue.markAsNoFollow,
} );

const newText = nextValue.title || newUrl;
Expand Down
15 changes: 14 additions & 1 deletion packages/format-library/src/link/modal-screens/link-settings-screen.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ const LinkSettingsScreen = ( {
const [ opensInNewWindow, setOpensInNewWindows ] = useState(
activeAttributes.target === '_blank'
);
const [ markedAsNoFollow, setMarkedAsNoFollow ] = useState(
( activeAttributes.rel = 'nofollow' )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Are there other options for this we need to consider?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@getdave getdave Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
const [ linkValues, setLinkValues ] = useState( {
isActiveLink: isActive,
isRemovingLink: false,
Expand All @@ -64,7 +67,7 @@ const LinkSettingsScreen = ( {
onHandleClosingBottomSheet( () => {
submit( inputValue, { skipStateUpdates: true } );
} );
}, [ inputValue, opensInNewWindow, text ] );
}, [ inputValue, opensInNewWindow, markedAsNoFollow, text ] );

useEffect( () => {
const { isActiveLink, isRemovingLink } = linkValues;
Expand Down Expand Up @@ -99,6 +102,7 @@ const LinkSettingsScreen = ( {
const format = createLinkFormat( {
url,
opensInNewWindow,
markedAsNoFollow,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
markedAsNoFollow,
markAsNoFollow: markedAsNoFollow,

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

text: linkText,
} );
let newAttributes;
Expand Down Expand Up @@ -213,6 +217,15 @@ const LinkSettingsScreen = ( {
onValueChange={ setOpensInNewWindows }
separatorType={ 'fullWidth' }
/>
<BottomSheet.SwitchCell
icon={ external }
label={ __(
'Search engines should ignore this link (mark as nofollow)'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

) }
value={ markedAsNoFollow }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

onValueChange={ setMarkedAsNoFollow }
separatorType={ 'fullWidth' }
/>
<BottomSheet.Cell
label={ __( 'Remove link' ) }
labelStyle={ styles.clearLinkButton }
Expand Down
19 changes: 17 additions & 2 deletions packages/format-library/src/link/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,17 @@ export function isValidHref( href ) {
* @param {string} options.type The type of the link.
* @param {string} options.id The ID of the link.
* @param {boolean} options.opensInNewWindow Whether this link will open in a new window.
* @param {boolean} options.markAsNoFollow Whether this link will be marked as nofollow.
*
* @return {Object} The final format object.
*/
export function createLinkFormat( { url, type, id, opensInNewWindow } ) {
export function createLinkFormat( {
url,
type,
id,
opensInNewWindow,
markAsNoFollow,
} ) {
const format = {
type: 'core/link',
attributes: {
Expand All @@ -101,7 +108,15 @@ export function createLinkFormat( { url, type, id, opensInNewWindow } ) {

if ( opensInNewWindow ) {
format.attributes.target = '_blank';
format.attributes.rel = 'noreferrer noopener';
format.attributes.rel = format.attributes.rel
? format.attributes.rel + ' noreferrer noopener'
: 'noreferrer noopener';
}

if ( markAsNoFollow ) {
format.attributes.rel = format.attributes.rel
? format.attributes.rel + ' nofollow'
: 'nofollow';
}

return format;
Expand Down