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

Sync this with the one in highlight.js #1

Open
chenglou opened this issue Sep 14, 2018 · 5 comments
Open

Sync this with the one in highlight.js #1

chenglou opened this issue Sep 14, 2018 · 5 comments

Comments

@chenglou
Copy link
Contributor

cc @gmmorris. Thanks for your work! I've tweaked the logic a bit and made sure that 99% of reason docs' snippet render correctly.

Building highlight.js master isn't always possible; case in point, reason's doc site uses Docusaurus, so highlightjs is a transitive dependency. Pinning it to master using yarn/npm wouldn't work either, because you'd get the un-built highlight.js (aka no lib/ folder).

For this reason, I've split out the highlight for easier integration and iteration. The highlightjs folks seem to want to eject the plugins too anyway.

In the meantime, we should sync up the one in the highlightjs repo with this one. Or simply remove the former. What's your opinion?

@gmmorris
Copy link

Hey,
I'm happy either way, I wasn't aware of the fork, and simply wanted better highlighting for my blog.

I'll try and take a look in the next couple of days, but if you want to sync it up and were concerned I might take offence (that my work is being replaced), then worry not. 😋

@chenglou
Copy link
Contributor Author

@gmmorris just made a few more improvements =)

Sorry to bother you, but have you seen highlightjs/highlight.js#1833 before? I'm trying to sync this back.

@gmmorris
Copy link

It's no bother, but sadly can't help, I haven't come across that.
It built well for me straight away, but my experience with highlights is limited to the reason work... Hadn't touched the project before that.

Good luck 😁

@joshgoebel
Copy link

joshgoebel commented Apr 24, 2021

[Highlight.js maintainer here] If this is the "canonical" grammar and maintained by ReasonML people then I'm inclined to remove the one we have in Highlight.js for many reasons:

  • It seems quite buggy and it has not really been maintained over time.
  • Lack of tests.
  • Complex "parser" based approach vs simpler pattern matching.
  • It uses lots of non-standard scopes that none of our themes support.

I was looking into fixing it, reducing the scopes, and bringing it up-to-date with the new work we're doing but if this already exists and all the ReasonML people in the know use it, then I think perhaps it should be removed from core entirely in favor of referring people here.

Thoughts?

@chenglou
Copy link
Contributor Author

Hey Josh. Yeah that sounds good. Thanks!

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

No branches or pull requests

3 participants