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

PLANET-4334 Allow a list of non-P4 urls not to have the ↗️ icon #1746

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

oekeur
Copy link
Contributor

@oekeur oekeur commented Jun 25, 2022

Summary

Adds the option to define a list of "Commonly used domains" under "Social" settings.
These domains should not get the external link icon and should always open in a new tab

As discussed in: greenpeace/planet4#174

Testing

  • Under "Social" > "Commonly used external domains" add https://www.google.com (or another variation of the url)
  • Test with content:
<!-- wp:paragraph -->
<p><a href="https://www.google.com">External link marked as commonly used</a><br>
<a href="https://www.google.com/android/find">Link with path marked as commonly used</a><br>
<a href="https://www.planet4.test/privacy-and-cookies/" data-type="page" data-id="232">Internal link</a> <br>
<a href="https://www.youtube.com/">External link NOT marked as commonly used</a></p>
<!-- /wp:paragraph -->

(or of course with any other content)

Parse list to make sure only hostnames are used
Thus removing the external link icon
Only consider the hostname
Change up a bit:
- Do not return early, so title is set
- Wrap adding the 'external-link' class in a conditional with commonExternalDomains
- Replace the target so commonExternalDomains always open in a new tab
@oekeur
Copy link
Contributor Author

oekeur commented Jul 7, 2022

Ping :) @lithrel @mleray

Copy link
Contributor

@lithrel lithrel left a comment

Choose a reason for hiding this comment

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

Just a condition change on links target 👌

if (!commonExternalDomains.includes(link.hostname)){
link.classList.add('external-link');
}
else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want external links to be the ones with target _blank

Copy link
Contributor Author

Choose a reason for hiding this comment

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

External links were getting _blank, but only the commonly used external domains, that indeed doesn't seem right 😄
At least solved that -for now- in 874a3e4 (#1746).

I'm doubting though.. Should commonly used domains maybe NOT be opened in _blank? ie GPNL uses an external domain for petitions, it might be beneficial to open that one in the same tab. Kinda edge casey though, so maybe not important for the generic case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm doubting though.. Should commonly used domains maybe NOT be opened in _blank? ie GPNL uses an external domain for petitions, it might be beneficial to open that one in the same tab. Kinda edge casey though, so maybe not important for the generic case?

So I'm realizing now that your PR says

These ("Commonly used") domains should not get the external link icon and should always open in a new tab

but the ticket says

Help text: "Links for these domains will not open in a new tab and will hide the external link icon."
Adjust external link behaviour to remove external link icon and opening in a new tab for these domains.

There might be a misunderstanding then ? From what I understand, links on Commonly used domains should open in the same tab.

Your last change ends up in a double assignement for link.target, I think this should work

if (commonExternalDomains.includes(link.hostname)) {
  // Commonly used domains: force open in same tab, no external link icon
  link.target = '';
} else {
  // Other domains: follow post config, add external link icon
  link.target = link.target ?? ''; // follow post config
  link.classList.add('external-link'); // external link icon
}

@mleray mleray requested a review from lithrel September 1, 2022 06:25
@oekeur
Copy link
Contributor Author

oekeur commented Jun 24, 2024

Hey @mleray ! Is this still relevant? 😅
I'll happily fix or close :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants