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 rel attribute for Social Icons #45469

Merged
merged 3 commits into from
Nov 2, 2022
Merged

Add rel attribute for Social Icons #45469

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52d7c1e
Add rel attribute for Social Icons
Nov 1, 2022
b94e848
Lint fix
Nov 1, 2022
15d9954
Cleanup
Nov 2, 2022
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
2 changes: 1 addition & 1 deletion docs/reference-guides/core-blocks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,7 @@ Display an icon linking to a social media profile or site. ([Source](https://git
- **Name:** core/social-link
- **Category:** widgets
- **Supports:** ~~html~~, ~~reusable~~
- **Attributes:** label, service, url
- **Attributes:** label, rel, service, url

## Social Icons

Expand Down
3 changes: 3 additions & 0 deletions packages/block-library/src/social-link/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
},
"label": {
"type": "string"
},
"rel": {
"type": "string"
}
},
"usesContext": [
Expand Down
18 changes: 16 additions & 2 deletions packages/block-library/src/social-link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
URLInput,
useBlockProps,
} from '@wordpress/block-editor';
import { Fragment, useState } from '@wordpress/element';
import { Fragment, useState, useCallback } from '@wordpress/element';
import {
Button,
PanelBody,
Expand Down Expand Up @@ -66,7 +66,7 @@ const SocialLinkEdit = ( {
isSelected,
setAttributes,
} ) => {
const { url, service, label } = attributes;
const { url, service, label, rel } = attributes;
const { showLabels, iconColorValue, iconBackgroundColorValue } = context;
const [ showURLPopover, setPopover ] = useState( false );
const classes = classNames( 'wp-social-link', 'wp-social-link-' + service, {
Expand All @@ -88,6 +88,13 @@ const SocialLinkEdit = ( {
},
} );

const onSetLinkRel = useCallback(
( value ) => {
setAttributes( { rel: value } );
},
[ setAttributes ]
);
Mamaduka marked this conversation as resolved.
Show resolved Hide resolved

return (
<Fragment>
<InspectorControls>
Expand All @@ -113,6 +120,13 @@ const SocialLinkEdit = ( {
</PanelRow>
</PanelBody>
</InspectorControls>
<InspectorControls __experimentalGroup="advanced">
<TextControl
label={ __( 'Link rel' ) }
value={ rel || '' }
onChange={ onSetLinkRel }
/>
</InspectorControls>
<li { ...blockProps }>
<Button
className="wp-block-social-link-anchor"
Expand Down
18 changes: 11 additions & 7 deletions packages/block-library/src/social-link/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ function render_block_core_social_link( $attributes, $content, $block ) {
$service = ( isset( $attributes['service'] ) ) ? $attributes['service'] : 'Icon';
$url = ( isset( $attributes['url'] ) ) ? $attributes['url'] : false;
$label = ( isset( $attributes['label'] ) ) ? $attributes['label'] : block_core_social_link_get_name( $service );
$rel = ( isset( $attributes['rel'] ) ) ? $attributes['rel'] : '';
$show_labels = array_key_exists( 'showLabels', $block->context ) ? $block->context['showLabels'] : false;

// Don't render a link if there is no URL set.
Expand All @@ -43,11 +44,6 @@ function render_block_core_social_link( $attributes, $content, $block ) {
$url = 'https://' . $url;
}

$rel_target_attributes = '';
if ( $open_in_new_tab ) {
$rel_target_attributes = 'rel="noopener nofollow" target="_blank"';
}

$icon = block_core_social_link_get_icon( $service );
$wrapper_attributes = get_block_wrapper_attributes(
array(
Expand All @@ -57,13 +53,21 @@ function render_block_core_social_link( $attributes, $content, $block ) {
);

$link = '<li ' . $wrapper_attributes . '>';
$link .= '<a href="' . esc_url( $url ) . '" ' . $rel_target_attributes . ' class="wp-block-social-link-anchor">';
$link .= '<a href="' . esc_url( $url ) . '" class="wp-block-social-link-anchor">';
$link .= $icon;
$link .= '<span class="wp-block-social-link-label' . ( $show_labels ? '' : ' screen-reader-text' ) . '">';
$link .= esc_html( $label );
$link .= '</span></a></li>';

return $link;
$w = new WP_HTML_Tag_Processor( $link );
$w->next_tag( 'a' );
if ( $open_in_new_tab ) {
$w->set_attribute( 'rel', esc_attr( $rel ) . ' noopener nofollow' );
$w->set_attribute( 'target', '_blank' );
} elseif ( '' !== $rel ) {
$w->set_attribute( 'rel', esc_attr( $rel ) );
}
return $w;
Copy link
Member

Choose a reason for hiding this comment

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

nice use of the tag processor!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed like a clearer solution than a bunch of string manipulation and worked like a charm!

Copy link
Member

@Mamaduka Mamaduka Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: While I also like how clean the "tag processor" looks here, but I don't think it is needed. We're building whole block output in the render callback; in cases like this, I would prefer PHP as a templating engine.

Copy link
Member

Choose a reason for hiding this comment

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

@Mamaduka I'm confused on what you are saying here. I guess I can see that we're constructing the link HTML entirely, we're not modifying content that was given to us, in which case we could change this whole thing to something like…

$anchor = $open_in_new_tab
	? '<a href="' . esc_url( $url ) . '" rel="' . esc_attr( $rel ) . ' noopener nofollow" target="_blank" class="wp-block-social-link">'
	: '<a href="' . esc_url( $url ) . '" class="wp-block-social-link">`;

…

$link .= $anchor;

that makes sense, though it does highlight a use-case for the tag processor that maybe wasn't the primary goal when creating it, which is making tag text construction more obvious. personally I often find these template pieces noisy and hard to follow, especially since they tend to make for very long lines with intertwining PHP and strings.

$anchor = new WP_HTML_Tag_Processor( "<a>" );
$anchor->next_tag( 'a' );
$anchor->set_attribute( 'href', $url );
$anchor->set_attribute( 'class', 'wp-block-social-link-anchor' );
if ( $open_in_new_tab ) {
	$anchor->set_attribute( 'rel', "{$rel} no opener nofollow" );
	$anchor->set_attribute( 'target', '_blank' );
} else {
	$anchor->set_attribute( 'rel', $rel );
}
$link .= $anchor;

now I think maybe there could be a good helper for this 🤔

static function make_tag_opener( $tag_name, $attributes = [] ) {
	$w = new WP_HTML_Tag_Walker( "<{$tag_name}>" );
	$w->next_tag( $tag_name );

	foreach ( $attributes as $name => $value ) {
		$w->set_attribute( $name, $value );
	}

	return $w;
}

this code here would then turn into something even less noisy, and at the same time, properly encoded.

$anchor = WP_HTML_Tag_Processor::make_tag_opener( 'a', array(
	'href'  => $url,
	'class' => 'wp-block-social-anchor',
	'rel'   => $rel
) );

if ( $open_in_new_tab ) {
	$anchor->set_attribute( 'rel', "{$rel} no opener nofollow" );
	$anchor->set_attribute( 'target', '_blank' );
}

…

$link .= $anchor;

none of this is to contradict your input; just taking the opportunity to muse on how this could evolve. this is a very DOM-like interface with its benefits as well as its drawbacks.

cc: @adamziel

Copy link
Member

Choose a reason for hiding this comment

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

@dmsnell, sorry about the confusion.

I guess I can see that we're constructing the link HTML entirely, we're not modifying content that was given to us, in which case we could change this whole thing to something like

This is what I was trying to formulate but failed 😅

}

/**
Expand Down