Skip to content

Related Posts: change default link rel value to empty #13637

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

Jump to bottom
Merged
merged 3 commits into from
Oct 24, 2019
Merged

Related Posts: change default link rel value to empty #13637

merged 3 commits into from
Oct 24, 2019

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Oct 3, 2019

Fixes #3366

Changes proposed in this Pull Request:

It isn't useful to block search engines from following links within your site. References:

Testing instructions:

  • Check related posts on your site.
  • Links should not include a nofollow rel attribute by default.

Proposed changelog entry for your changes:

  • Related Posts: remove nofollow attribute from links.

@jeherve
Related Posts: change default link rel value to empty
cc307ae 
Fixes #3366

It isn't useful to block search engines from following links within your site. References:
- https://www.youtube.com/watch?v=4FkSZIW6d48#t=1852
- https://www.youtube.com/watch?v=4SAPUx4Beh8
@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Related Posts [Status] Needs Review This PR is ready for review. [Pri] Low labels Oct 3, 2019
@jeherve jeherve added this to the 7.9 milestone Oct 3, 2019
@jeherve jeherve requested a review from a team October 3, 2019 15:43
@jeherve jeherve self-assigned this Oct 3, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D33542-code before merging this PR. Thank you!

@jeherve
Copy link
Member Author

jeherve commented Oct 3, 2019

@chrisfromthelc Wanna jump in here? I'd be happy to have a second opinion on this.

Thank you!

@jetpackbot
Copy link
Collaborator

jetpackbot commented Oct 3, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against a15a610

kraftbj
kraftbj previously approved these changes Oct 10, 2019
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

This works and produced valid HTML.

Would it make sense to have including the rel in the output conditional if there is a value? (L290-ish) or would that make the output creation a bit overly complex relative to the benefits?

I'm fine with an empty rel, so approving.

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 10, 2019
@chrisfromthelc
Copy link
Contributor

@chrisfromthelc Wanna jump in here? I'd be happy to have a second opinion on this.

Sorry, I just saw this! I would prefer to omit rel completely if there's no value, even though technically an empty value is valid HTML. That is a preference, and definitely splitting hairs on this for sure.

@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D33542-code has been updated.

@jeherve
Copy link
Member Author

jeherve commented Oct 11, 2019

Would it make sense to have including the rel in the output conditional if there is a value?

Good call. Done in 7e5af5e for both the block and the regular related posts.

This should be good for another review now.

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 11, 2019
@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D33542-code has been updated.

ChaosExAnima
ChaosExAnima approved these changes Oct 23, 2019
View reviewed changes
Copy link
Contributor

@ChaosExAnima ChaosExAnima left a comment

Choose a reason for hiding this comment

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

Looks good here 👍

@ChaosExAnima ChaosExAnima added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Oct 23, 2019
@jeherve jeherve merged commit f181d8b into master Oct 24, 2019
@jeherve jeherve deleted the update/related-posts-default-dofollow branch October 24, 2019 08:44
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Oct 24, 2019
jeherve added a commit that referenced this pull request Oct 28, 2019
@jeherve
Changelog: add #13637
c513f64
jeherve added a commit that referenced this pull request Oct 29, 2019
@jeherve
7.9: Changelog (#13608)
6cd1621 
* 7.9: Changelog

* Update version number

* Update stable tag and tested up to

* Changelog: add #13530

* changelog: add #13578

* Changelog: add #13598

* Changelog: add entry for numerous block preview changes

* Changelog: add #13599

* changelog: add #13541

* Changelog: add #13542

* Changelog: add #13331

* Changelog: add #13558

* Changelog: add #13409

* Changelog: add #13582

* Changelog: add #13600

* Changelog: add #13601

* Changelog: add #13595

* Changelog: add #12695

* Changelog: add #13009

* Changelog: add #13649

* Changelog: add #13450

* Changelog: add #13507

* Changelog: add #13658

* Changelog: add #13687

* changelog: add #13683

* Changelog: add #9323

* Changelog: add #13681

* Fix typos in readme

* Add link to WordPress Beta Tester plugin

* Changelog: add #13630

* Changelog: add #13695

* Changelog: add #13659

* Changelog: add #13716

* Changelog: add #13664

* Changelog: add #13682

* Changelog: add #13362

* Changelog: add #13563

* Add testing list for #13563

* Changelog: add #13735

* Changelog: add #13752

* Changelog: add #13624

* Changelog: add #13756

* Changelog: add #13745

* Changelog: add #13728

* Changelog: add #13779

* Changelog: add #13699

* Changelog: add #13804

* Changelog: add #13761

* Changelog: add #13637

* Changelog: add #13517

* Changelog: add #13521

* Changelog: add #13729

* Testing list: add testing instructions for #13729

* Changelog: add sync changes

* Changelog: add #13807

* Changelog: add #13654

* Changelog: add #13795

* Changelog: add #13801

* Changelog: add #13818

* Changelog: add #13725

* Changelog: add #13831

* Changelog: add #13516

* Testing list: add Twenty Twenty instructions

* Changelog: add #13799

* Changelog: add #13805

* Changelog: add #13688

* Changelog: add #13830
@lschuyler lschuyler mentioned this pull request Jul 31, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj left review comments

+1 more reviewer

@ChaosExAnima ChaosExAnima ChaosExAnima approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@jeherve jeherve

Labels
[Feature] Related Posts [Pri] Low Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
7.9
Development

Successfully merging this pull request may close these issues.

Related Posts: make posts dofollow by default
6 participants
@jeherve @matticbot @jetpackbot @chrisfromthelc @kraftbj @ChaosExAnima