Skip to content
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

[ready to merge] Fixes #84 #85

Merged
merged 9 commits into from
Aug 31, 2020
Merged

[ready to merge] Fixes #84 #85

merged 9 commits into from
Aug 31, 2020

Conversation

mesqueeb
Copy link
Contributor

@mesqueeb mesqueeb commented May 3, 2020

No description provided.

@mesqueeb
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

mesqueeb commented May 16, 2020

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
Copy link
Contributor Author

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

@mesqueeb
Copy link
Contributor Author

@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
Copy link
Owner

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 mesqueeb changed the title Fixes #84 [ready to merge] Fixes #84 May 19, 2020
@mesqueeb
Copy link
Contributor Author

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

TLDR;

  • double checked with PrismJS CDN usage, works like a charm. (the intent of this PR)
  • If this gets merged, will make a new PR to update the Readme with a short guide on PrismJS highlighting (the intent of this PR)
  • the only file over 1000bytes is snarkdown.umd.js.gz, see image:

image

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

@mesqueeb
Copy link
Contributor Author

@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
Copy link
Owner

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.

@developit developit merged commit 64806f1 into developit:master Aug 31, 2020
@mesqueeb mesqueeb deleted the patch-1 branch September 1, 2020 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants