When youtube shouldn't embed
-
FORKED:
@boomzilla said in Youtube thumbnails:
update the regex for finding a youtube url and then using it to embed it in the markup.
We should also make sure it's the first and only thing on the line to prevent unwanted oneboxing. And probably also differentiate between a raw link pasted in and when someone links some text or whatever to the youtube url. Blakey was (correctly) complaining about this somewhere.
-
@boomzilla said in Youtube thumbnails:
Blakey was (correctly) complaining
-
-
@boomzilla Did you add "fucking DUH!" to the bug description?
If you guys want a good baseline of a bug so obvious that it shows the author of the software didn't even spend 10 seconds testing their own code, that'd be it.
-
@blakeyrat No. Feel free to add a comment on github.
-
@boomzilla I actually wouldn't have really minded adding a onebox under the paragraph (keeping the link intact), but ERASING the paragraph and REPLACING it with the onebox, that's super-special.
-
@blakeyrat It might not be the worst idea to always just add it after the paragraph always. But that's a bit more complicated than what I did, which was to make sure it was just the link inside opening and closing paragraph tags.
-
@boomzilla Will that change always work? There might be whitespace between the
<p>
opener and closer and the contained<a>
-
@boomzilla Your fix works for me.
But I'm still mind-boggled that this got released in such a broken state in the first place. The bottom of the software quality barrel moves down another notch.
-
@RaceProUK Hmm...it works with extra whitespace. But requires a linebreak in between. So...I guess a better solution would include a check for a
<br>
.
-
@boomzilla I'm trying your fix out now. If the entire contents of the post is this:
[Watch this video](https://www.youtube.com/watch?v=8NAJWPuZFTk)
... the whole post is still replaced by the embedded video.
-
@NedFodder That's not the fix, that's the broken.
-
@NedFodder Ugh...
-
@blakeyrat His code does fix this:
Watch [this](https://www.youtube.com/watch?v=8NAJWPuZFTk) video.
-
OK...this regex seems to fix that issue (plus works with only a single newline between URLS):
/(?:<p>|^)<a.*?href="((https?:\/\/www\.)?youtube\.com\/\S*(?:(?:\/e(?:mbed))?\/|watch\?(?:\S*?&?v\=))|(https?:\/\/)?youtu\.be\/)([a-zA-Z0-9_-]{6,11})"[^>]*?>\1\4<\/a>(?:<br\/?>|<\/p>)/mg;
-
-
@NedFodder Oh I see what you mean now.
-
BTW...I found some other issues...and I'm....writing tests currently. Which also requires that I figure out how to use mocha / chai (because that's what I started using). Have them mostly working now, I think. I'll probably push them later today.
Shit...detached HEAD. Fucking git.
-
@boomzilla said in Youtube thumbnails:
Which also requires that I figure out how to use mocha / chai (because that's what I started using).
There's a few of here with experience with both of them (like the SockDevs), so if you have any questions, feel free to keep them to yourself :P
Nah, seriously, we'd be happy to help ;)
-
@RaceProUK said in Youtube thumbnails:
There's a few of here with experience with both of them (like the SockDevs)
Indeed. Hearing you guys talk about those packages were what lead me to them. And I took a peek at some sock stuff to figure some things out. :-)
-
OK!
- detects linked text
- regex tests using mocha + chai
$ npm test > nodebb-plugin-youtube-lite@0.5.0 test /home/boomzilla/youtube-boomzilla > mocha ./tests regex tests Simple post with only the youtube link ✓ converts the post correctly Markdown link with a youtube URL should be left alone ✓ converts the post correctly Video URLs on consecutive lines ✓ converts the post correctly Link with text in front should be left alone ✓ converts the post correctly Link with text behind should be left alone ✓ converts the post correctly Link with text in front and behind should be left alone ✓ converts the post correctly 6 passing (8ms)
-
-
@Luhmann said in Youtube thumbnails:
Guess that's karma.
What did I do to deserve this?
Anyways...I just cloned a new repo and copied over my changes. Seemed a lot easier for this tiny repo and the 3 files that I had touched.
-
-
@boomzilla said in Youtube thumbnails:
detects linked text
regex tests using mocha + chaiWoah... is that work you've done there?
-
@loopback0 said in Youtube thumbnails:
@boomzilla said in Youtube thumbnails:
detects linked text
regex tests using mocha + chaiWoah... is that work you've done there?
Damnit, @boomzilla! Now you're setting a bad precident!
-
@abarker said in Youtube thumbnails:
prec
iedent
-
@RaceProUK Precident's daughter gone bad.
-
So...these are the cases that I have in my tests (the tests themselves deal with html, since that's what the plugin gets):
Simple post with only the youtube link
https://youtu.be/fXhUgV9qzI0
Markdown link with a youtube URL should be left alone
[linked](https://youtu.be/fXhUgV9qzI0)
Video URLs on consecutive lines
https://youtu.be/fXhUgV9qzI0 https://youtu.be/fXhUgV9qzI0
Link with text in front should be left alone
Look at this https://youtu.be/fXhUgV9qzI0
Link with text behind should be left alone
https://youtu.be/fXhUgV9qzI0 uh huh
Link with text in front should be left alone
Look: https://youtu.be/fXhUgV9qzI0 uh huh
Did I miss anything? (NB: stuff like
?t=30s
is more complicated and not intended to be supported by this change)
-
Pull requested:
-
@boomzilla Is there a reason every describe has one and only one it?
Usually you'd do something like
describe("The regex parser", () => { it("Should convert simple posts"), () => { /*test*/}); it("Should not convert markdown links"), () => { /*test*/}); it("Should convert URLs on consecutive lines"), () => { /*test*/}); it("Should leave the text in front of a link alone"), () => { /*test*/}); it("Should leave the text behind a link alone"), () => { /*test*/}); it("Should leave surrounding text alone"), () => { /*test*/}); });
-
@Yamikuronue said in When youtube shouldn't embed:
Is there a reason every describe has one and only one it?
Actually, there's a describe with a loop that has a describe / it combination.
I dunno...the combination made sense to me at the time and I was working with a blank slate when it comes to mocha / chai conventions. And the
{} => {}
sort of stuff is not in my toolbox, so there's no way that would have happened.
-
@boomzilla said in When youtube shouldn't embed:
there's a describe with a loop that has a describe / it combination.
Ew.
Yeah, I saw that later. You're still generating code that looks like what I posted (save the ES6 stuff, that
(() => {}
is just shorthand forfunction() {}
)
-
@Yamikuronue said in When youtube shouldn't embed:
save the ES6 stuff, that (() => {} is just shorthand for function() {} )
Dammit. I was just starting to get used to
function(){}
.
-
@boomzilla said in When youtube shouldn't embed:
@Yamikuronue said in When youtube shouldn't embed:
save the ES6 stuff, that (() => {} is just shorthand for function() {} )
Dammit. I was just starting to get used to
function(){}
.you can still keep using that if you want.
might be a good idea to avoid ES6 stuff while working on nodebb actually as nodebb still supports nodejs 0.12 without the
--harmony
flag and so must be pure ES5 (core at least. i'm sure you could come up with a rasin to have a plugin need ES6 if you really wanted to)
-
I'll just leave this here:
Apparently @Lorne-Kates decided to link the full URL by hand, and Onebox picked it up.
-
@Tsaukpaetra said in When youtube shouldn't embed:
I'll just leave this here:
Apparently @Lorne-Kates decided to link the full URL by hand, and Onebox picked it up.
No, Lorne clicked the "attach photo by URL" button and then clicked the "attach photo by upload" button.
-
@ben_lubar said in When youtube shouldn't embed:
No, Lorne clicked the "attach photo by URL" button and then clicked the "attach photo by upload" button.
Wat.
Speaking of, which one of those upload buttons are we going to be eradicating (soon)?
-
@Tsaukpaetra was there ever supposed to be a difference? I never quite figured that out.
-
@anotherusername the cloud-shaped one opens an image upload dialog and the one on the right opens a file upload dialog. The image upload dialog also accepts files that aren't images.
-
@ben_lubar said in When youtube shouldn't embed:
The image upload dialog also accepts files that aren't images.
Does the file upload dialog also accept images that aren't files?
-
@FrostCat I haven't seen one of those, but if you can fit one into your computer, probably.
-
@ben_lubar well, they both open an
All Files (*.*)
upload box for me.
The icons are unintuitive though. Intuitively, those icons should mean "upload from URL" and "upload from disk". But they both upload from disk. And they completely fail to convey the fact that one of them is for uploading files, and the other is just for uploading images.
@FrostCat said in When youtube shouldn't embed:
Does the file upload dialog also accept images that aren't files?
Both buttons work for either images or files with exactly the same result regardless of which button you clicked.
-
@ben_lubar said in When youtube shouldn't embed:
@FrostCat I haven't seen one of those,
I know Linux sucks, but it doesn't have a clipboard?
-
@blakeyrat everything is a file on Linux. Even your CPU is a file.
-
@ben_lubar said in When youtube shouldn't embed:
@blakeyrat everything is a file on Linux.
So when you hit control-C, the data gets written to a file somewhere? I find that difficult to believe.
-
@blakeyrat I'm not sure if SIGINT is a file.
-
@ben_lubar said in When youtube shouldn't embed:
I'm not sure if SIGINT is a file.
Someone just told me it was. Who was it... they typed this:
@ben_lubar said in When youtube shouldn't embed:
everything is a file on Linux. Even your CPU is a file.
OH YEAH it was you.
-
@blakeyrat oh, apparently it is a file on cygwin: http://williammitchell.blogspot.com/2008/03/fun-with-cygwins-devclipboard.html
-
@ben_lubar I'm going to take that as an admission that the clipboard is not a file in Linux and you are wrong and I am right and someone give me a fucking laurel already here, sheesh.