gregjacobs / Autolinker.js

Utility to Automatically Link URLs, Email Addresses, Phone Numbers, Twitter handles, and Hashtags in a given block of text/HTML
MIT License
1.48k stars 238 forks source link

Skip correct URLs #24

Closed cycology77 closed 10 years ago

cycology77 commented 10 years ago

Does this script skip over existing correct HTML URLs?

gregjacobs commented 10 years ago

Yes, it should, but let me know if you find a situation where it doesn't.

cycology77 commented 10 years ago

Think I'm running into a case where it's affecting manual links: http://www.wgxa.tv/story/mcdaniel-pleads-guilty-to-lauren-giddings-murder-20140421

About half way into the story there's a link: Download McDaniel's full confession, that's not clickable. The source code is correct: McDANIEL'S FULL CONFESSION --> Download McDaniel's full confession

The site is built on the dotCMS platform. The code I'm using to get the script to run is:

(script src="http://www.wgxa.tv/includes/js/Autolinker.min.js") (/script) (script) $("#storyBody p").each(function() { $(this).html(Autolinker.link($(this).text())); }); (/script)

(tags were getting removed from comment)

If I remove the script from the page the link works as expected.

gregjacobs commented 10 years ago

Hey, so sorry for the late reply. Can you send the section of html that doesn't seem to be autolinked correctly? (or in this case, html that doesn't seem to be skipped over correctly?)

Thanks, Greg

cycology77 commented 10 years ago

No worries, I really appreciate your help with this. So here's a link to a page that works: http://www.wgxa.tv/story/man-fatally-shot-at-party-in-stone-mountain-20140505

Here's the page where the link was constructed correctly in our CMS, but no longer works once I activate Autolinker.js: http://www.wgxa.tv/story/mcdaniel-pleads-guilty-to-lauren-giddings-murder-20140421

The source for the section including the link:

McDANIEL'S FULL CONFESSION --> Download McDaniel's full confession.

screen shot 2014-05-05 at 2 57 17 pm

Let me know if you need any more info from me! Thanks again.

gregjacobs commented 10 years ago

Hey, give it a try now with v0.9.1. The HTML parsing has been updated a bit, which might fix the issue.

If not, another thing that comes to mind is to make sure that you have valid markup relating to opening and closing <a> tags. Autolinker tries its best, but isn't a full HTML parser and might have trouble if there are <a> tags that aren't explicitly closed. (Which, as a side note, also made me start thinking about what to do with named anchors...).

Either way, let me know :)

cycology77 commented 10 years ago

Thanks Greg, - trying it out now!

cycology77 commented 10 years ago

Unfortunately no luck with the updated code. I started playing around with a few of the options in the script as well as turning off all other JS to make sure there wasn't a conflict somewhere. I noticed few things in my testing.

If the text is set to a URL instead of any anchor text, everything works as expected. Great! As soon as there's tags involved though, the link is broken (just like in our original example).

This was unexpected though: If I set urls : false in the script any text URLs are ignored, as expected, but any correctly coded URLs are still broken!

I thought maybe there was something else going on with another script that might be causing this, but I tested again after disabling all JS but Autolinker on the page and my download URL was still broken. If I disable Autolinker and reload the page the link works again.

gregjacobs commented 10 years ago

Hey cycology. I think I see the problem. You seem to be calling Autolinker by passing in the inner text of each of your <p> tags, rather than passing their inner html.

$("#storyBody p").each(function() {
    $(this).html(Autolinker.link($(this).text()));
});

.text() will return the contents of your <p> tags with all nested tags stripped, so your original tags would be gone before even calling Autolinker. You'll want to change that $(this).text() to $(this).html()

Try that and let me know how it goes.

cycology77 commented 10 years ago

Beautiful!

Thank you so much for your help with this!

On Thu, May 8, 2014 at 12:35 PM, Gregory Jacobs notifications@github.comwrote:

Hey cycology. I think I see the problem. You seem to be calling Autolinker by passing in the inner text of each of your

tags, rather than passing their inner html.

$("#storyBody p").each(function() { $(this).html(Autolinker.link($(this).text()));});

.text() will return the contents of your

tags with all nested tags stripped, so your original tags would be gone before even calling Autolinker. You'll want to change that $(this).text() to $(this).html()

Try that and let me know how it goes.

— Reply to this email directly or view it on GitHubhttps://github.com/gregjacobs/Autolinker.js/issues/24#issuecomment-42572579 .

All my best,

-c

Chris Hood

478.954.0042 | c www.chrislhood.com

gregjacobs commented 10 years ago

Awesome, glad it's working! Enjoy!