marky-markdown icon indicating copy to clipboard operation
marky-markdown copied to clipboard

Published 20 hours ago • verified publisher icon npm

solved #274 review youtube embed handling

Open shibasisp opened this issue 9 years ago • 7 comments

is this okay?

shibasisp avatar Oct 26 '16 02:10 shibasisp

can anyone please tell me what is the problem? Why the tests are failing?

shibasisp avatar Oct 26 '16 03:10 shibasisp

Hey @shibasisp, that was quick! Fantastic!

Guess what? I totally forgot to list that we have unit tests around this as well, and for TravisCI to pass the PR, the youtube unit tests need to be removed as well. Oops. Those are in the test directory.

However, @ashleygwilliams brought up a good point in #274, that the ideal outcome here might be different than just removing the youtube stuff, because there are definitely users (the npm site, at least) that need it. So if you don't mind, I have to hang on to this until she's able to investigate a bit more and we can decide where the youtube processing stuff needs to live.

revin avatar Oct 26 '16 03:10 revin

Ok fine..

shibasisp avatar Oct 26 '16 05:10 shibasisp

hi! so yes- until i can completely move the npm docs off of marky-markdown (it's in the plan, just not a priority at the moment) we'll need to keep this functionality. that being said, because it is not inline with the product direction of parity with github-flavored markdown, i would be happy to see it moved behind a flag as part of the deprecation path! cc/@revin

ashleygwilliams avatar Oct 26 '16 14:10 ashleygwilliams

That sounds good. So @shibasisp does that sound interesting to you? Basically add an option in index.js, say, allowDeprecatedYoutubeEmbeds: false (unless you have any particular preference for the name, @ashleygwilliams), and only .use(youtube) in render.js if the option is true, similar to how the syntax highlighting and CDN things work.

Then instead of removing the tests, they'd need updated so the current tests pass if the option is set to true, and add some new tests to verify that the <iframe>s are stripped out when the option is set to false or omitted.

Is that enough information for you to know what changes to make? I'm totally happy to help if anything is confusing. 👍

revin avatar Oct 26 '16 15:10 revin

no preference on name for the flag!

ashleygwilliams avatar Nov 04 '16 15:11 ashleygwilliams

Hey again @shibasisp, do you have the time/availability to implement the version of this where it's deprecated and not removed entirely? If not, I can take over from here if you like. No pressure. Thanks again!

revin avatar Nov 07 '16 17:11 revin