Jump to conversation
Unresolved conversations (5)
@laurent22 laurent22 Apr 23, 2020
If you use a url parser you no longer need your regex. Just print the result of the url.parse call and you'll see you have all the elements that you need.
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
Rishabh-malhotraa laurent22
@laurent22 laurent22 Apr 11, 2020
Thanks for the detailed tests. It also need to work with these cases: - `[some title here](http://www.youtube.com/watch?v=iwGFalTRHDA)` => should create a video player and display a link "some title here" below the video, which opens the video on YouTube. - `http://www.youtube.com/watch?v=iwGFalTRHDA` => should create a video player - `https://www.a.com/watch?v=SADub7W22Zg` => should **not** create a video player (it currently does!)
Outdated
CliClient/tests/md_to_html/video.md
Rishabh-malhotraa
@laurent22 laurent22 Apr 11, 2020
For the description please use this: > Converts video URLs into a video player. Currently only YouTube is supported. More information in [this post](https://github.com/laurent22/joplin/pull/2747).
Outdated
readme/markdown.md
@laurent22 laurent22 Apr 11, 2020
Please prefix the class names with "jop" eg `.jop-video-container`. This is because notes can contain arbitrary HTML, especially those imported from web clipper, and so we add a prefix to avoid any conflict.
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
@laurent22 laurent22 Apr 11, 2020
Please fix the indentation (tabs only)
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
Resolved conversations (15)
@laurent22 laurent22 Apr 23, 2020
checkHostName => isValidHostname
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
@laurent22 laurent22 Apr 11, 2020
This works on mobile too as far as I can see so please enable it in this case too.
Outdated
ReactNativeClient/lib/models/Setting.js
Rishabh-malhotraa
@laurent22 laurent22 Apr 11, 2020
This regex is faulty, for examples it matches this, but shouldn't: `'https://www.a.com/watch?v=SADub7W22Zg'.match(/^.*(youtu\.be\/|v\/|u\/\w\/|embed\/|watch\?v=|&v=)([^#&?]*).*/)`
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
Rishabh-malhotraa laurent22
@laurent22 laurent22 Apr 11, 2020
- Please remove the width and height as that should be controlled by your CSS now. - Please remove the double/triple spaces for example around `class="video-iframe"` or `frameborder="0"`
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
@laurent22 laurent22 Apr 11, 2020
What do we need this padding for?
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
Rishabh-malhotraa
@laurent22 laurent22 Mar 23, 2020
`videoID[2].length == 11` is not necessary if your regex is correct.
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
Rishabh-malhotraa PackElend
laurent22
@laurent22 laurent22 Mar 23, 2020
Please could you change this to use only one regex. I think videoIDRegex should already give the ID so the `key` doesn't seem necessary.
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
Rishabh-malhotraa
@laurent22 laurent22 Mar 23, 2020
Please move this to the `plugins` object below because your plugin is setup like a regular markdown-it plugin, unlike the rules in this object.
...iveClient/lib/joplin-renderer/MdToHtml.js
Rishabh-malhotraa
@laurent22 laurent22 Mar 23, 2020
Please use a template literal to format and indent the HTML string - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals Any reason why you chose width 560? I guess it makes sense to set a min and max width, but otherwise could you make it shrink and grow with the container? Could you document these properties and are they necessary? `accelerometer; autoplay; encrypted-media; gyroscope; picture-in-picture` For example, for me the video doesn't even auto-play (and I don't think it should) so the autoplay property maybe is not needed. I don't know about the others.
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
sinkuu Rishabh-malhotraa
laurent22
@laurent22 laurent22 Mar 15, 2020
Please don't silence exceptions, it leads to poor quality software since errors get undetected and never get fixed. Secondly, the way I understand this code is like this: if the user adds a YouTube URL, display a video player. If they add any other URL, display nothing. Or am I missing something? You didn't add automated-tests yet but did you test with regular URLs in the app?
Outdated
...b/joplin-renderer/MdToHtml/rules/video.js
Rishabh-malhotraa laurent22
@miciasto miciasto Mar 14, 2020
You've got a small typo. Perhaps: 'Enable video embedding'
Outdated
ReactNativeClient/lib/models/Setting.js
Rishabh-malhotraa
@laurent22 laurent22 Mar 13, 2020
Normally this should be implemented in TypeScript, as mention in CONTRIBUTING.md. But let's say in JS is fine too, provided the implementation is correct.
Outdated
...lin-renderer/MdToHtml/rules/embedvideo.js
@laurent22 laurent22 Mar 13, 2020
rule should be called "video", not "embedvideo".
Outdated
...iveClient/lib/joplin-renderer/MdToHtml.js
@laurent22 laurent22 Mar 13, 2020
"tokenize" means converting a string to a stream of tokens. That's not what this function does.
Outdated
...lin-renderer/MdToHtml/rules/embedvideo.js
Rishabh-malhotraa
@laurent22 laurent22 Mar 13, 2020
That solution is not optimum as you're unnecessarily parsing the whole Markdown document even though it's already been parsed by markdown-it. Markdown-it will produce a stream of tokens, which you can hook into to provide alternative rendering. In your case, you could hook in link_open and, if you detect that the href is a YouTube URL, display your iframe code. Please have a look at the markdown-it doc for more info.
Outdated
...lin-renderer/MdToHtml/rules/embedvideo.js
Rishabh-malhotraa