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

fix: allows no rel attribute on external links in the nav #2338

Merged
merged 5 commits into from Jul 23, 2020
Merged

fix: allows no rel attribute on external links in the nav #2338

merged 5 commits into from Jul 23, 2020

Conversation

abea
Copy link
Contributor

@abea abea commented Apr 23, 2020

Summary

Currently it's not possible to specify that there should be no rel attribute on external nav links to use the default behavior. Setting to an empty string, as indicated in the documentation, is falsy, so you end up in the final ternary, which uses noopener noreferrer for external links.

This change allows devs to pass in a falsy, non-undefined, value to the rel property to have no rel attribute on the link. It also change the fallback of an empty string to null since an empty rel attribute has no particular meaning, so it's better to have it not there at all.

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

I could not tell from the contributor guide or the package scripts how to build this for linking to a project and testing. I'd be happy to do that with some direction.

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

Copy link
Collaborator

@billyyyyy3320 billyyyyy3320 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@@ -69,14 +69,14 @@ module.exports = {
}
```

Outbound links automatically get `target="_blank" rel="noopener noreferrer"`. You can offer `target` and `rel` to customize the attributes:
Outbound links automatically get `target="_blank" rel="noopener noreferrer"`. You can offer `target` and `rel` to customize the attributes. Passing a "falsy" value to the `rel` attribute will disable that attribute for a link:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, maybe I should make this clearer to say that the falsy value can't be undefined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO, change the rule. Let user set false rather than falsy value to disable the attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense.

@abea abea requested a review from billyyyyy3320 May 2, 2020 10:58
@abea
Copy link
Contributor Author

abea commented May 4, 2020

Should I create an issue to update this in the Chinese docs or something?

@billyyyyy3320
Copy link
Collaborator

Should I create an issue to update this in the Chinese docs or something?

I'll take it later. No worries.

Co-authored-by: Billyyyyy3320 <newsbielt703@gmail.com>
@abea abea requested a review from billyyyyy3320 May 4, 2020 17:37
Copy link
Collaborator

@billyyyyy3320 billyyyyy3320 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bencodezen bencodezen left a comment

Choose a reason for hiding this comment

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

Thanks for your PR @abea!

@bencodezen bencodezen merged commit b343cd3 into vuejs:master Jul 23, 2020
@abea abea deleted the allow-no-rel branch July 23, 2020 19:08
larionov pushed a commit to larionov/vuepress that referenced this pull request Aug 19, 2020
* fix: allows no rel attribute on external links in the nav

* fix: documents the falsy rel option

* fix: clarifies documentation

* fix: explicitly uses  for rel attr

* fix: removes unnecessary condition

Co-authored-by: Billyyyyy3320 <newsbielt703@gmail.com>

Co-authored-by: Billyyyyy3320 <newsbielt703@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants