-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Support target="_blank"
for <router-link>
#830
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know how to use this GitHub review. Should have made it a comment instead :P
// don't redirect if `target="_blank"` | ||
/* istanbul ignore if */ | ||
const target = this.$el && this.$el.attributes.getNamedItem('target') | ||
if (target && target.value === '_blank') return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
- Why not
const target = this.$el && this.$el.getAttribute('target')
? - Won't this fail for
target="_blank "
(with an extra space)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, I guess I'm just too unfamiliar with native dom api 😂
// don't redirect if `target="_blank"` | ||
/* istanbul ignore if */ | ||
const target = this.$el && this.$el.getAttribute('target') | ||
if (target && target.toLowerCase().split(' ').indexOf('_blank') > -1) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since code is written in ES Net, shouldn't it be target.toLowerCase().split(' ').includes('_blank')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That cannot be transpiled, and would require a polyfill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about just /\b_blank\b/i.test(target)
@fnlctrl I absolutely forgot about this, sorry |
@posva Totally forgot this too 😂😂😂 |
@@ -49,6 +49,11 @@ export default { | |||
// don't redirect on right click | |||
/* istanbul ignore if */ | |||
if (e.button !== 0) return | |||
// don't redirect if `target="_blank"` | |||
/* istanbul ignore if */ | |||
const target = this.$el && this.$el.getAttribute('target') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use e.target
here. this.$el
may be a wrapping <li>
if tag
prop is used.
// don't redirect if `target="_blank"` | ||
/* istanbul ignore if */ | ||
const target = this.$el && this.$el.getAttribute('target') | ||
if (target && target.toLowerCase().split(' ').indexOf('_blank') > -1) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about just /\b_blank\b/i.test(target)
@yyx990803 , hello you da, but how can I do it in the instance, like this:
|
If it's supported you should update this line. |
No description provided.