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 linksInNewTab config option #1030

Closed
wants to merge 2 commits into from

Conversation

ocdtrekkie
Copy link

All credit due to @drmuey, reference is #659 for his work. This should merge against the current codebase.

@joshbruce joshbruce added this to the 0.5.0 - Architecture and extensibility milestone Jan 22, 2018
@joshbruce
Copy link
Member

Why not post the Issue instead of a PR as was suggested?

This still doesn't comply with the specs.
Still doesn't have tests.
And doesn't fit with the focus of the community right now.

Flagged for 0.5.0 at the moment.

@ocdtrekkie
Copy link
Author

You wanted a big green button. I am looking at how to add a test and will add it to this branch.

@joshbruce
Copy link
Member

Taking 'em one at a time. I appreciate the zeal. Let's see where it goes. :)

@joshbruce
Copy link
Member

I should note, we haven't fully fleshed out the extensibility approach; so, not sure how much will change by the time we actually get to this...or if we will, to be honest. The more Marked takes on, the more chance there is for Marked to be in error...none of us are paid to maintain the code; so, it is in all of our best interests to keep it small - see #964

@ocdtrekkie
Copy link
Author

I wrangled the testing setup a bit, the Linux VM I had handy is... outdated. I got an NPM error before and after adding my test, but after tweaking it a bit (I missed adding the <p> tags originally), it shows as completed.

@ocdtrekkie
Copy link
Author

ocdtrekkie commented Jan 22, 2018

I completely understand the desire for brevity. I believe this PR is very brief, and it touches an extremely small amount of code. I believe the likelihood of this PR leading to bugs is small. The only thing I can think of that someone will need to be aware of, is that if code is added to add any other properties to the <a> structure, they will need to have a leading space, as this code does (and the title tag also does), before the new property. It should coexist with any new properties added similarly to itself.

@styfle
Copy link
Member

styfle commented Jul 10, 2018

I think you would want to add a few additional attributes to avoid cross-origin vulnerabilities such as rel=noopener. Also what about rel=nofollow? It's hard to know what the user wants in this scenario.

@ocdtrekkie
Copy link
Author

@styfle The original PR for this feature was created in 2015. I have pretty much abandoned all hope for marked at this point.

@UziTech
Copy link
Member

UziTech commented Jul 11, 2018

I think this feature would be better as an extension. it shouldn't be too hard to provide a renderer that does this.

@styfle
Copy link
Member

styfle commented Jul 11, 2018

@UziTech Agreed, there are too many variables here that it doesn't make sense to be in core.

@styfle styfle closed this Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants