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

Jump to bottom

Adds 'nofollow' setting to Button block #54110

Merged
merged 9 commits into from
Sep 21, 2023
Merged

Adds 'nofollow' setting to Button block #54110

merged 9 commits into from
Sep 21, 2023

Conversation

bangank36
Copy link
Contributor

@bangank36 bangank36 commented Sep 1, 2023 •
edited
Loading

What?

Add the ability to toggle the nofollow for Button's link.

Partial implementation of #54091
Prior art for Rich text link #53945

Why?

Check #53945 (comment)

How?

  • Place the nofollow option in the Advanced settings area
  • Update the linkValue for persisting the Advanced settings state

Testing Instructions

  • Open Post Editor
  • Add new Button block
  • Add a link to the Button
  • Click Edit link
  • Toggle the opensInNewTab and nofollow
  • Check and confirm whether the rel and target attributes has been inserted correctly
opensInNewWindow nofollow Generated
✅ ❌ target="_blank" rel="noreferrer noopener"
❌ ✅ target="" rel="nofollow"
✅ ✅ target="_blank" rel="noreferrer noopener nofollow"
❌ ❌ target="" rel=""

Testings

  • Verify and update these e2e tests
    npm run test:e2e:playwright -- test/e2e/specs/editor/blocks/links.spec.js
    npm run test:e2e:playwright -- test/e2e/specs/editor/blocks/buttons.spec.js

  • Insert new unit testing for updateLinkAttributes
    npm run test:unit -- packages/block-library/src/button

Testing Instructions for Keyboard

Screenshots or screencast

Kapture.2023-09-01.at.19.39.59.mp4

cc: @getdave

@getdave getdave added [Block] Buttons Affects the Buttons Block [Type] Feature New feature to highlight in changelogs. [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) labels Sep 1, 2023
@bangank36 bangank36 marked this pull request as ready for review September 2, 2023 01:50
@bangank36 bangank36 force-pushed the add/nofollow-for-button-link branch from f22ca0c to 3137c93 Compare September 2, 2023 06:12
alexstine
alexstine reviewed Sep 3, 2023
View reviewed changes
Copy link
Contributor

@alexstine alexstine left a comment

Choose a reason for hiding this comment

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

One note below.

packages/block-editor/src/components/link-control/index.js Outdated Show resolved Hide resolved
@getdave getdave mentioned this pull request Sep 5, 2023
Closed
@scruffian
Copy link
Contributor

This is looking good, just needs a rebase I think.

getdave

This comment was marked as outdated.

Sign in to view

getdave
getdave previously requested changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@getdave getdave left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm going to block this PR because I"m not confident that we should be adding to the API of this component.

Let's discuss this further and get a wider range of opinions before we proceed.

Thank you 🙇

packages/block-editor/src/components/link-control/index.js Outdated Show resolved Hide resolved
@bangank36 bangank36 force-pushed the add/nofollow-for-button-link branch from 8ae1504 to 08d44c6 Compare September 8, 2023 02:58
@bangank36 bangank36 requested a review from getdave September 8, 2023 02:59
@bangank36 bangank36 requested a review from getdave September 13, 2023 00:01
@bangank36 bangank36 force-pushed the add/nofollow-for-button-link branch from 2626796 to dbe9b5f Compare September 14, 2023 03:38
@bangank36
Copy link
Contributor Author

Hi @getdave, just a friendly nudge to request your review of the latest changes on my PR when you have a moment. Thanks!

getdave
getdave reviewed Sep 20, 2023
View reviewed changes
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

This is very close. Thank you for your patience.

I think upon reflection some unit tests would help add confidence here. They will also lay a solid foundation for a later refactoring.

packages/block-library/src/button/edit.js Outdated Show resolved Hide resolved
@bangank36 bangank36 force-pushed the add/nofollow-for-button-link branch from 6df0f71 to 7895ebd Compare September 21, 2023 03:56
@bangank36 bangank36 requested a review from getdave September 21, 2023 04:07
getdave
getdave reviewed Sep 21, 2023
View reviewed changes
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Great work here. A few more tweaks and we should be in a good place.

packages/block-library/src/button/utils.js Outdated Show resolved Hide resolved
packages/block-library/src/button/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/button/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/button/edit.js Outdated Show resolved Hide resolved
@bangank36 bangank36 requested a review from getdave September 21, 2023 09:26
getdave
getdave approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Thank you for all the work on this.

LGTM 🚀

@getdave
Copy link
Contributor

getdave commented Sep 21, 2023

Note to WP 6.4 leads. I don't believe we need to backport this PR. Happy to be advised otherwise if folks find it to be sufficiently "blessed".

@getdave getdave merged commit 191fd0f into WordPress:trunk Sep 21, 2023
49 checks passed
@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 21, 2023
@bangank36 bangank36 mentioned this pull request Sep 21, 2023
Open
This was referenced Sep 25, 2023
Merged
Draft
@skorasaurus skorasaurus mentioned this pull request Jun 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexstine alexstine alexstine left review comments

@getdave getdave getdave approved these changes

@kevin940726 kevin940726 Awaiting requested review from kevin940726

@Mamaduka Mamaduka Awaiting requested review from Mamaduka

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@ellatrix ellatrix Awaiting requested review from ellatrix

@fluiddot fluiddot Awaiting requested review from fluiddot

@dcalhoun dcalhoun Awaiting requested review from dcalhoun

@scruffian scruffian Awaiting requested review from scruffian

Assignees

@bangank36 bangank36

Labels
[Block] Buttons Affects the Buttons Block [Feature] Link Editing Link components (LinkControl, URLInput) and integrations (RichText link formatting) [Type] Feature New feature to highlight in changelogs.
Projects
Automattic team Ignite's project board
Archived in project
Milestone
Gutenberg 16.8
Development

Successfully merging this pull request may close these issues.
4 participants
@bangank36 @scruffian @getdave @alexstine