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.

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=nofollow to the links. #77

Closed
wants to merge 1 commit into from

Conversation

marksabbath
Copy link
Contributor

Should fix #59

@marksabbath
Copy link
Contributor Author

Hey, @nickcernis I think that adding rel="nofollow" to all the links would be the best approach, right?

Would you mind reviewing this?

Thanks in advance :)

Copy link
Contributor

@nickcernis nickcernis left a comment

Choose a reason for hiding this comment

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

@marksabbath Are we sure that everyone will want nofollow?

I thought the same as you at first but it looks like some people prefer not to have nofollow on their social media links.

Maybe a filter instead?

	/**
	 * Construct the markup for each icon
	 *
	 * @param string $icon The lowercase icon name for use in tag attributes.
	 * @param string $label The plain text icon label.
	 *
	 * @return string The full markup for the given icon.
	 */
	function get_icon_markup( $icon, $label ) {
		$markup = '<li class="ssi-' . $icon . '"><a href="%s" %s>';
		$markup .= '<svg role="img" class="social-' . $icon . '" aria-labelledby="social-' . $icon . '-{WIDGET_INSTANCE_ID}">';
		$markup .= '<title id="social-' . $icon . '-{WIDGET_INSTANCE_ID}">' . $label . '</title>';
		$markup .= '<use xlink:href="' . esc_url( plugin_dir_url( __FILE__ ) . 'symbol-defs.svg#social-' . $icon ) . '"></use>';
		$markup .= '</svg></a></li>';

		return apply_filters( 'simple_social_icon_html', $markup );
	}

That way people can do this in their theme if they want to:

add_filter( 'simple_social_icon_html', 'custom_social_icon_html' );
function custom_social_icon_html( $html ) {
	return str_replace( '<a', '<a rel="nofollow"', $html );
}

@marksabbath
Copy link
Contributor Author

@nickcernis what about adding a new checkbox to allow the user to have control in the WordPress backend, without a necessity of adding any extra code?

@nickcernis
Copy link
Contributor

@marksabbath I checked help desk for other requests for nofollow in Simple Social Icons, but couldn't find anything. Since we don't see many requests for it we might not need the checkbox just yet.

Do you think a filter would suffice for now? One plus is it also allows people to add 'rel=me' and other attributes if they want.

@davidcolden
Copy link

davidcolden commented Sep 4, 2018

Sorry I'm late to comment on this...

I feel it would also be good if we could have "noopener" included as well on the rel if the setting is to open the links in a new window.

This is for a number of reasons...

  • the new page runs on the same process as your page so If the new page is executing expensive / large javascript calls potentially the calling page's performance will also be impacted...

  • opening links (with target="_blank") is also a security vulnerability as the new page has access to your window object via window.opener.

This would also potentially give an uplift from a Google perspective as it's one of their best practices for improving quality in a site.

@nickcernis
Copy link
Contributor

Thanks for your feedback, @davidcolden. You bring up a good point, and I've opened a new issue for this (#78) to track in a future update, since it's more closely related to the “open links in new window” behaviour than the nofollow being discussed here.

People could potentially add noopener via the above filter if they wanted to in the meantime, though.

@marksabbath
Copy link
Contributor Author

Closing this one. Filter implemented on #79.

@marksabbath marksabbath closed this Sep 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add filter to the markup output
3 participants