-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Refactors embedded youtube link regex #1155
Conversation
Just some detailed explanation of the issue that is fixed by this pull request: Youtube link differ for its mobile version: 'https://m.youtube.com/...'. So when such link is pasted into video embed tooltip an error is thrown:
Steps for Reproduction
Expected behavior:
Actual behavior: Platforms: Version: 1.1.5 |
So mobile link like 'http://m.youtube.com/...' can be embedded
Can you include tests? This regex is getting unwieldy with additional forms of YouTube links it is expected to support. |
This is based on StackOverflow: var reg = /^.*(youtu.be\/|v\/|u\/\w\/|embed\/|watch\?v=|\&v=)([a-zA-Z0-9_-]+).*/;
var match = value.trim().match(reg);
if (match && match[2].length == 11) {
value = 'https://www.youtube.com/embed/' + match[2] + '?showinfo=0';
} |
@jhchen Hi, could you please tell where you would like these tests to be in |
@jhchen could you please answer the question above? It's critical for me to have this and I can finish the pull request (i.e., add tests) |
In terms of file organization it should probably go in theme/base.js since that is what is being tested. There may need to be some refactoring of the source code to enable unit testing which may influence which test/ folder or file it goes into. |
+1 😃 We're currently running into this issue. Is there anything blocking this merge? |
As a workaround you can create own theme which extends SnowTheme and monkey patch Tooltip. This is what I did |
This allows to convert m.youtube.com and www.youtube.com to embedded url as well Finishes work of slab#1155
@ygrishajev you can close this. The work was finished in #1489 |
So mobile link like 'http://m.youtube.com/...' can be embedded