developit / snarkdown

:smirk_cat: A snarky 1kb Markdown parser written in JavaScript
http://jsfiddle.net/developit/828w6t1x/
MIT License
2.29k stars 109 forks source link

[ready to merge] Fixes #84 #85

Closed mesqueeb closed 4 years ago

mesqueeb commented 4 years ago

Hi Jason (@developit)

Do you have any opinion on this change? If all is OK, I can update the tests as well to prepare for merge.

Thank you for your time.

developit commented 4 years ago

This looks good to me! If you've got the time to add tests that'd be great, though I'm fine merging this since its a fairly straightfoward fix.

mesqueeb commented 4 years ago

Hi @developit Thanks for giving me a few days to adjust the update the tests as well.

You'll see that all tests now pass again with the new <pre><code> syntax.

Also, I've updated microbundle to latest version, because building failed otherwise.

And I've also added the language-${token} class to the <pre> tag. (further reading on the reasoning here and here)

Looking forward to your deploy! 🎉

mesqueeb commented 4 years ago

(nvm that last commit. prismJS needs it to be on the <pre> tag to work)

mesqueeb commented 4 years ago

@developit it seems that adding a class to <pre> makes the build fail because it's too large. Shall I remove it again? This is required for prismJS highlighting to work, but is something I could add manually with a .replace() as well...

As a matter of fact, it's possible to manually replace even the <pre> with <pre><code> as well, making this PR useless. 😅

developit commented 4 years ago

Hey @mesqueeb - that is for all your work on this. I will check it out locally, but if you know how much the size increased that'd be useful.

mesqueeb commented 4 years ago

@developit How've you been? I hope you're doing well!

TLDR;

image

What's your policy on a single build file over 1000b?

mesqueeb commented 4 years ago

@developit Since you already approved this PR, are you feeling OK to work with me towards merging this?

I'm having some issues with dependencies of mine that have to use my fork from github that clash with other dependencies that are using the main package.

developit commented 4 years ago

Hi @mesqueeb - sorry about the delay, I'll try to get this dealt with today. I think it's good to go, just need to check a couple things and do the release.