-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add rel attribute for Social Icons #45469
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
Size Change: +510 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no problem with this, unless someone finds the "Link rel" label confusing. If that's the case it's easy to improve it as we go.
} else if ( $rel !== '' ) { | ||
$w->set_attribute('rel', esc_attr( $rel) ); | ||
} | ||
return $w; |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, @georgeh!
I left two suggestions, but feel free to ignore them.
@dmsnell, the 'Link rel' will make this consistent with other blocks. There's also a suggestion to add a description to the link relationship settings. See #20208 (comment). |
When adding "me" or any other text in the rel field, it does not remove "noopener nofollow". So the rel attribute now looks like this: rel="me noopener nofollow". Wouldn't I want Google to follow the link if I am adding "me"?
|
What?
Makes
rel
attribute editable on social icons, closes #38630Why?
Some social networks like IndieWeb and Mastodon use the pattern of looking for a link back with
rel="me"
to establish that a user controls a URL. Adding the option for a customrel
attribute allows people to claim their WordPress sites using Gutenberg.How?
This mimics the implementation of the
rel
attribute on the Button block, by exposing the attribute as an advanced option in the block inspector.Testing Instructions
rel
rel
attribute on the<a>
elementScreenshots or screencast