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

Make linkify links open in new tab/window #214

Merged
merged 4 commits into from
Aug 4, 2016
Merged

Make linkify links open in new tab/window #214

merged 4 commits into from
Aug 4, 2016

Conversation

mentos1386
Copy link
Contributor

I believe this behavior is more appropriate.

I believe this behavior is more appropriate.
@vincentwoo
Copy link
Contributor

vincentwoo commented Aug 3, 2016

I think the appropriateness of this flag can vary wildly by application. Here's a random opinion piece on the topic: https://css-tricks.com/use-target_blank/.

I think setting target=_blank by default is fine, but it would be cool if you could toggle it off.

@parisk
Copy link
Contributor

parisk commented Aug 3, 2016

Hi @mentos1386, thanks a lot for your contribution! I think it fits the needs of xterm.js a lot, based on its main use cases. @vincentwoo is right though that the appropriateness of this flag can vary and if #198 was closed it would definitely help.

What I would like to see here in order to move with merging is making this an option, by adding an additional argument to the linkify and linkifyTerminalLine called target that will default to null and if set it would set the link's target attribute to the given value.

At this moment it is important not to break the current behavior of xterm.js.

You can now provide target argument if you want to modify target attribute on links. It defaults to `_self` to keep current behavior.
@mentos1386
Copy link
Contributor Author

I changed it so that you can provide argument and it will set target to what you provide. Default behavior is that it doesn't change anything.

@@ -67,6 +69,8 @@
throw new TypeError(message);
}

typeof target === 'undefined' ? target = '' : target = 'target="' + target + '"';
Copy link
Contributor

@parisk parisk Aug 4, 2016

Choose a reason for hiding this comment

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

Could you please turn this in the following form to make it more human understandable?

if (typeof target === 'undefined') {
  target = '';
} else {
  target = 'target="' + target + '"';
}

EDIT: Please check for typos, I typed it really quickly to provide you the big picture 😅 !

@parisk
Copy link
Contributor

parisk commented Aug 4, 2016

Works great, thanks @mentos1386 🎉 !

@parisk parisk merged commit c333cfa into xtermjs:master Aug 4, 2016
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.

3 participants