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

adds nofollow and blank target #16640

Closed
wants to merge 1 commit into from
Closed

adds nofollow and blank target #16640

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
4 changes: 4 additions & 0 deletions packages/block-library/src/navigation-menu-item/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@
"destination": {
"type": "string"
},
"rel": {
Copy link
Member

Choose a reason for hiding this comment

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

It seems the (block) attribute rel can be computed from the attributes opensInNewTab and nofollow. If that's the case can we remove the rel attribute, and on the save function depending on the attributes opensInNewTab and nofollow we save the current value for the rel (HTML attribute)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but why? Isn't rel an attribute anyway? Is there a reason to have less attributes, especially when we are actually printing attributes on render?

Copy link
Member

Choose a reason for hiding this comment

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

The reason is that each time we update the original attribute we also need to update the derived attribute forgetting to do that introduces bugs. In the future, one may easily add a logic that changes nofollow attribute and forgets to change the rel attribute. Another reason is that saving an attribute that can be computed adds more information in the database without a need.

"type": "string",
"default": ""
},
"nofollow": {
"type": "boolean",
"default": false
Expand Down
31 changes: 23 additions & 8 deletions packages/block-library/src/navigation-menu-item/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { invoke } from 'lodash';
import classnames from 'classnames/dedupe';

/**
* WordPress dependencies
Expand Down Expand Up @@ -39,13 +40,30 @@ function NavigationMenuItemEdit( {
setAttributes,
} ) {
const plainTextRef = useRef( null );
const onEditLableClicked = useCallback(
const onEditLabelClicked = useCallback(
( onClose ) => () => {
onClose();
invoke( plainTextRef, [ 'current', 'textarea', 'focus' ] );
},
[ plainTextRef ]
);

const setTarget = ( opensInNewTab ) => {
const { rel } = attributes;

const updatedRel = classnames( rel, { noopener: opensInNewTab, noreferrer: opensInNewTab } );

setAttributes( { opensInNewTab, rel: updatedRel } );
};

const setNofollow = ( nofollow ) => {
const { rel } = attributes;

const updatedRel = classnames( rel, { nofollow } );

setAttributes( { nofollow, rel: updatedRel } );
};

let content;
if ( isSelected ) {
content = (
Expand Down Expand Up @@ -74,7 +92,7 @@ function NavigationMenuItemEdit( {
<MenuItemActions
clientId={ clientId }
destination={ attributes.destination }
onEditLableClicked={ onEditLableClicked( onClose ) }
onEditLableClicked={ onEditLabelClicked( onClose ) }
/>
) }
/>
Expand All @@ -83,6 +101,7 @@ function NavigationMenuItemEdit( {
} else {
content = attributes.label;
}

return (
<Fragment>
<InspectorControls>
Expand All @@ -91,9 +110,7 @@ function NavigationMenuItemEdit( {
>
<ToggleControl
checked={ attributes.opensInNewTab }
onChange={ ( opensInNewTab ) => {
setAttributes( { opensInNewTab } );
} }
onChange={ setTarget }
label={ __( 'Open in new tab' ) }
/>
<TextareaControl
Expand All @@ -117,9 +134,7 @@ function NavigationMenuItemEdit( {
/>
<ToggleControl
checked={ attributes.nofollow }
onChange={ ( nofollow ) => {
setAttributes( { nofollow } );
} }
onChange={ setNofollow }
label={ __( 'Add nofollow to menu item' ) }
help={ (
<Fragment>
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/navigation-menu-item/save.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ export default function save( { attributes } ) {
return (
<a
href={ attributes.destination }
rel={ attributes.nofollow && 'nofollow' }
rel={ attributes.rel }
title={ attributes.title }
target={ attributes.opensInNewTab && '_blank' }
>
Expand Down