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

youtu.be short links are very popular. #147

Closed
wants to merge 1 commit into from
Closed

Conversation

ser
Copy link

@ser ser commented Oct 22, 2015

It could probably fit into single line, but I am not a javascript expert. Anyway, youtu.be links are very popular and should be added.

@mweibel
Copy link
Member

mweibel commented Oct 22, 2015

Instead of an additional line you could replace

youtube.com\/watch\?v=

With

(youtube.com\/watch\?v=|youtu.be\/)

You'd probably need to change the $1/$2 etc. as well then, because you introduce another matching group. A good resource for testing regexes is: https://regex101.com/ (you can switch to JS mode)

Also please have a look at the indenting, you changed the indenting :)

@Sudrien
Copy link
Contributor

Sudrien commented Oct 22, 2015

It could be one-lined, but the syntax is different enough it would be much harder to debug the regex.

Two suggestions:

  • Force https in your iframes. There's really no reason not to take the https option these days.
  • Look into preserving start time, if specified. There's an option to set it when generating a share link. I think it's t=

@Sudrien Sudrien closed this Oct 22, 2015
@Sudrien Sudrien reopened this Oct 22, 2015
@Sudrien
Copy link
Contributor

Sudrien commented Oct 22, 2015

Thanks, mobile interface.

@mweibel
Copy link
Member

mweibel commented Oct 22, 2015

@Sudrien: I think it's perfectly fine to use the protocol-independent // TBH, but it doesn't really matter (and isn't related to the PR).
Also, adding t= - not related to the PR, would be a welcome addition though.

@Sudrien
Copy link
Contributor

Sudrien commented Oct 30, 2015

As there have been no further movement -
@ser - If you want to just fix the spacing, I'll look into t= support after this is merged

@ser
Copy link
Author

ser commented Oct 30, 2015

I really do not understand what do you want, so I'm closing it.

@ser ser closed this Oct 30, 2015
@Sudrien
Copy link
Contributor

Sudrien commented Oct 31, 2015

@ser - Your commit converted the indentation from tabs to spaces. It's a little easier to see at https://github.com/candy-chat/candy-plugins/pull/147/files then in your text editor, probably.

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.

None yet

3 participants