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 hostname white list to skip adding rel attributes #45

Open
kenchan0130 opened this issue Oct 15, 2019 · 12 comments
Open

Add hostname white list to skip adding rel attributes #45

kenchan0130 opened this issue Oct 15, 2019 · 12 comments
Assignees
Labels
enhancement New feature or request question Further information is requested

Comments

@kenchan0130
Copy link

This issue was moved from kenchan0130#10.

The behavior I'm seeking is:

  • All external links open in a new window
  • All external links get noopner and noreferrer
  • An optional white list of hostnames can be added which don't get the rel nofollow attributes (even if set)

Use case:

I'm going to be adding a site where 95% of the links are external and 5% of the links are trusted so I don't want to set nofollow.

Potential API for usage:

target-blank:
  rel_nofollow_skip_domains: ["example.com", "github.com"]

Example link output:

<a href="http://example.com" target="_blank">Example</a>
<a href="https://example.com" target="_blank">Example</a>
<a href="https://www.example.com" target="_blank">Example</a>
<a href="http://foo.github.com" target="_blank">GitHub</a>
<a href="https://github.com" target="_blank">GitHub</a>
<a href="https://foo.bar.github.com" target="_blank">GitHub</a>
<a href="https://somewhere.com" target="_blank" rel="nofollow">Somewhere</a>
@keithmifsud keithmifsud added enhancement New feature or request question Further information is requested labels Oct 16, 2019
@keithmifsud
Copy link
Owner

Thanks, @kenchan0130 :)

I understand the issue and I'm more than happy to add it on. Have you got a PR for this or you have not started working on it yet?

@kenchan0130
Copy link
Author

@keithmifsud I haven't started work yet.

@keithmifsud
Copy link
Owner

Okies, I'm happy to help you with this feature if you get stuck. It is quite straight forward to implement. However, usage wise, you will need to be careful. In your example, you listed github.com (I know it is just an example), your site will probably have external links to GH. Some f which will to repos you manage and some to repos others manage. This can harm your site's SEO.

@kenchan0130
Copy link
Author

kenchan0130 commented Oct 18, 2019

@keithmifsud
I recognize that this library currently doesn't support the nofollow grant itself.
IMO, I think it is necessary to consider the advantages and disadvantages of the following choices.

  1. Whether to support the nofollow attribute
    • I would say nofollow attribute allows us to choose whether to trust external sites. So, "Some f which will to repos you manage and some to repos others manage." is an individual problem of the site, and I think the scope of the discussion is different.
  2. What to do with the default behavior, with support for nofollow attribute. Whether to be selective to end users in config.
    • If this library can be selected on site, the same request will be created with the noreferrer attribute. Therefore, we may want to be careful in supporting these features.

@keithmifsud
Copy link
Owner

keithmifsud commented Oct 18, 2019

Hi @kenchan0130 ,

Currently, you can still add nofollow and any other rel value:

target-blank:
    rel: nofollow

However, as you stated, it is not configurable, at least yet 😄

I mean you cannot select which links to add the nofollow to. Just all external links or all links with the predefined CSS class.

@kenchan0130
Copy link
Author

We may do the following:

target-blank:
   noopener:
       allow: ["regexp string or pure string"]
       ignore: ["regexp string or pure string"]
   noreferrer:
       allow: ["regexp string or pure string"]
       ignore: ["regexp string or pure string"]
   nofollow:
       allow: ["regexp string or pure string"]
       ignore: ["regexp string or pure string"]

In other words, I propose a method like noopener, noreferrer and nofollow attributes of config support Boolean and Object with Array of String types.

In addition, rel attribute of config may be excluded.

@keithmifsud
Copy link
Owner

I like the idea but I think that the regexp is hard for most and very hard to test against. If you can build the feature in a TDD manner than, please do :)

Please ensure existing functionality remain intact as this library is used by more 10K installs and we should respect no BC.

@jmankoff
Copy link

jmankoff commented Jan 5, 2020

I would love to have this too!

@kenchan0130
Copy link
Author

kenchan0130 commented Jan 7, 2020

If you adopt my proposal, I don't know what to do with rel atrribute.
What behavior do you expect if settings are enabled separately for the rel attribute and individual attributes?

@keithmifsud
Copy link
Owner

If you adopt my proposal, I don't know what to do with rel atrribute.
What behavior do you expect if settings are enabled separately for the rel attribute and individual attributes?

Hi @kenchan0130 is this message meant for me or for @jmankoff ?

@kenchan0130
Copy link
Author

Hi @keithmifsud, this message is mainly for you.

@keithmifsud
Copy link
Owner

Thanks @kenchan0130 :) I don't think I understand the question? You can already set the rel attribute as a separate setting if one wishes to customise its behaviour.

nicks added a commit to tilt-dev/tilt.build that referenced this issue Dec 7, 2021
The jekyll-target-blank's definition of an "external" site is
non-customizable, and it doesn't seem like the upstream maintainers
are interested in fixing this.
keithmifsud/jekyll-target-blank#45
nicks added a commit to tilt-dev/tilt.build that referenced this issue Dec 7, 2021
The jekyll-target-blank's definition of an "external" site is
non-customizable, and it doesn't seem like the upstream maintainers
are interested in fixing this.
keithmifsud/jekyll-target-blank#45
nicks added a commit to tilt-dev/tilt.build that referenced this issue Dec 8, 2021
)

The jekyll-target-blank's definition of an "external" site is
non-customizable, and it doesn't seem like the upstream maintainers
are interested in fixing this.
keithmifsud/jekyll-target-blank#45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants