Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

馃帹 Add wp-link attribute to all links #81

Merged
merged 3 commits into from
Oct 17, 2022

Conversation

SantosGuillamot
Copy link
Contributor

I'm opening this Pull Request to "Add the wp-link to all the relative links on the page", one of the tasks of the Tracking issue: WordPress Directives Plugin. This PR aims to just add that attribute in PHP, while all the logic will be handled within the directives in JS.

The first idea is to handle that in the render_block filter using the WP_HTML_Tag_Processor.

@SantosGuillamot SantosGuillamot changed the base branch from main-custom-elements-hydration to main-wp-directives-plugin October 17, 2022 12:45
@SantosGuillamot
Copy link
Contributor Author

So far, I just added a single filter, but I encountered two problems:

  • If there are two links in the same block, the wp-link attribute is only added to the first element. For example, a paragraph with two links:
<p>First Link: <a wp-link="true" href="/">First Link</a>. Second Link: <a href="/sample-page">Second Link</a>.</p>

I haven't triaged it yet, but any ideas why this is happening or how to solve it are welcome 馃檪

@luisherranz luisherranz changed the title Add wp-link attribute to all links 馃帹 Add wp-link attribute to all links Oct 17, 2022
@luisherranz
Copy link
Member

Oh, sorry. The HTML Tag Processor needs to be called recursively:

$w = new WP_HTML_Walker( $html );
while ( $w->next_tag("a") ) {
  // ...
}

By the way, I've just noticed that in one of the examples of the Make Core post, they are using $w->get_attribute("sr"). Maybe it's already implemented. Could you please check it out?

@SantosGuillamot
Copy link
Contributor Author

The HTML Tag Processor needs to be called recursively

That makes sense 馃槃 I've just changed it.

By the way, I've just noticed that in one of the examples of the Make Core post, they are using $w->get_attribute("sr"). Maybe it's already implemented. Could you please check it out?

Yes, it seems implemented! 馃檪 I added a check for the hostname and another one to exit the loop if target === 'blank'. Any more use cases where we shouldn't add the wp-link attribute?

I will try to take a look now at why this isn't working in the navigation block using directives.

@luisherranz
Copy link
Member

Any more use cases where we shouldn't add the wp-link attribute?

We can add more later if we need to 馃檪

Copy link
Member

@luisherranz luisherranz 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 to me, Mario!

wp-directives.php Show resolved Hide resolved
wp-directives.php Outdated Show resolved Hide resolved
@SantosGuillamot SantosGuillamot marked this pull request as ready for review October 17, 2022 15:10
@SantosGuillamot
Copy link
Contributor Author

Thanks for the review! Changes addressed 馃檪

@SantosGuillamot SantosGuillamot merged commit 67aa9f5 into main-wp-directives-plugin Oct 17, 2022
@SantosGuillamot SantosGuillamot deleted the add-wp-link-attribute branch October 17, 2022 15:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants