feat: Loom video support #105
Conversation
|
@radiovisual Thoughts on this PR? It would be great to have loom support in the library. |
|
Loom support sounds nice. I can take a closer look at the PR soon |
There was a problem hiding this comment.
@Dheerajkawatra Thanks for the contribution! Sorry for the super late review!
| * https://www.loom.com/share/7c7ced4911904070a5627374ccd84e8c | ||
| * https://www.loom.com/share/5bbdeb480ba84e65b1b3de8c190e2003?source=embed&t=20 | ||
| * https://www.loom.com/embed/7c7ced4911904070a5627374ccd84e8c |
There was a problem hiding this comment.
I try to keep links to real videos out of the documentation
| * https://www.loom.com/share/7c7ced4911904070a5627374ccd84e8c | |
| * https://www.loom.com/share/5bbdeb480ba84e65b1b3de8c190e2003?source=embed&t=20 | |
| * https://www.loom.com/embed/7c7ced4911904070a5627374ccd84e8c | |
| * https://www.loom.com/share/{id} | |
| * https://www.loom.com/share/{id}? | |
| * https://www.loom.com/embed/{id} | |
| * https://www.loom.com/embed/{id}? |
|
|
||
| test('Loom embed', () => { | ||
| expect(fn('<div style="position: relative; padding-bottom: 62.5%; height: 0;"><iframe src="https://www.loom.com/embed/7c7ced4911904070a5627374ccd84e8c" frameborder="0" webkitallowfullscreen mozallowfullscreen allowfullscreen style="position: absolute; top: 0; left: 0; width: 100%; height: 100%;"></iframe></div>').id).toBe('7c7ced4911904070a5627374ccd84e8c'); | ||
| }); |
There was a problem hiding this comment.
Can you also add a test to be sure that the id is extracted from the embed URL that also has query parameters? For example: https://www.loom.com/embed/123?foo=true => 123
| https://www.loom.com/share/7c7ced4911904070a5627374ccd84e8c | ||
| https://www.loom.com/share/5bbdeb480ba84e65b1b3de8c190e2003?source=embed&t=20 |
There was a problem hiding this comment.
I try to keep links to real videos out of the documentation
| https://www.loom.com/share/7c7ced4911904070a5627374ccd84e8c | |
| https://www.loom.com/share/5bbdeb480ba84e65b1b3de8c190e2003?source=embed&t=20 | |
| https://www.loom.com/share/* | |
| https://www.loom.com/share/*? |
|
|
||
| **Loom iframe** | ||
| ``` | ||
| <div style="position: relative; padding-bottom: 62.5%; height: 0;"><iframe src="https://www.loom.com/embed/7c7ced4911904070a5627374ccd84e8c" frameborder="0" webkitallowfullscreen mozallowfullscreen allowfullscreen style="position: absolute; top: 0; left: 0; width: 100%; height: 100%;"></iframe></div> |
There was a problem hiding this comment.
| <div style="position: relative; padding-bottom: 62.5%; height: 0;"><iframe src="https://www.loom.com/embed/7c7ced4911904070a5627374ccd84e8c" frameborder="0" webkitallowfullscreen mozallowfullscreen allowfullscreen style="position: absolute; top: 0; left: 0; width: 100%; height: 100%;"></iframe></div> | |
| <iframe src="https://www.loom.com/embed/*" ... |
| export default function loom(urlString) { | ||
| // Parse basic url and embeds | ||
| const basicReg | ||
| = /(https?:\/\/)?(www\.)?loom\.com\/?(.*\/)?([\d)([a-z]{32})\??.*/; |
There was a problem hiding this comment.
Seems like this RegEx is way to strict which might present problems (false positives, not flexible, etc), so I think you can really simplify this RegEx to make it focus on the specific strings you are looking for, and don't force length limits on the hashed id. something like (this is not tested, so please test it or modify it accordingly):
| = /(https?:\/\/)?(www\.)?loom\.com\/?(.*\/)?([\d)([a-z]{32})\??.*/; | |
| = /^https?:\/\/(?:www\.)?loom\.com\/(?:share|embed)\/([\da-zA-Z]+); |
no worries, Thanks for the review @radiovisual . I'll fix asap |
|
@Dheerajkawatra are you still interested in this PR? I can pick up the remaining work if you need some help. |
|
I will close this stale PR since Loom video support was added via #110 |
No description provided.