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.

Sign up for GitHub

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

Jump to bottom

Replace prettify.js with highlight.js #1578

Merged
merged 1 commit into from
Oct 28, 2020
Merged

Replace prettify.js with highlight.js #1578

merged 1 commit into from
Oct 28, 2020

Conversation

zakame
Copy link
Contributor

@zakame zakame commented Oct 17, 2020

Summary

This replaces the old prettify.js highlighter with highlight.js.

Motivation

prettify.js has long been abandoned by Google so we need to replace it.

That said, highlight.js is also somewhat dormant (highlightjs/highlight.js#1678) but still sees enough activity to get around.

References

This is an update of #737, fixes #1544.

Most of the work was already done by @dotandimet, please give credit where due. I just updated it for latest Mojo and highlight.js 💪

@kraih
Copy link
Member

kraih commented Oct 17, 2020 •
edited
Loading

The commit history is all messed up. Ideally we want one squashed commit for the whole feature.

kraih
kraih previously requested changes Oct 17, 2020
View reviewed changes
Copy link
Member

@kraih kraih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocked until the commit history is fixed.

@zakame zakame requested a review from kraih October 17, 2020 19:52
@mergify mergify bot dismissed kraih’s stale review October 17, 2020 19:52

Pull request has been modified.

@zakame
Copy link
Contributor Author

zakame commented Oct 17, 2020

@kraih thanks, I rebased against latest master now and squashed it all to a single commit 💪

@zakame zakame force-pushed the highlightjs branch 2 times, most recently from 80ae299 to b857dde Compare October 17, 2020 21:02
@zakame zakame requested a review from kraih October 17, 2020 21:36
@zakame zakame mentioned this pull request Oct 18, 2020
Merged
@kraih kraih requested a review from a team October 18, 2020 11:55
kraih
kraih previously approved these changes Oct 18, 2020
View reviewed changes
Copy link
Member

@kraih kraih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks much better now.

lib/Mojolicious/resources/public/mojo/highlight.js/highlight-mojo-dark.css Outdated Show resolved Hide resolved
@kraih kraih requested a review from a team October 18, 2020 16:51
@mergify
Copy link
Contributor

mergify bot commented Oct 19, 2020

This pull request is now in conflicts. Could you fix it @zakame? 🙏

@zakame zakame requested review from kraih and removed request for a team October 19, 2020 03:18
jhthorsen
jhthorsen reviewed Oct 19, 2020
View reviewed changes
Copy link
Member

@jhthorsen jhthorsen left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good change in general, but I am curious if we need to support all the languages in highlight.js/highlight.js.min. Limiting the supported languages to bash, css, diff, http, javascript, json, makefile, xml, perl, plaintext, shell, and sql slims down the file from 100k to 45k. Maybe we can limit it even further?

@zakame
Copy link
Contributor Author

zakame commented Oct 19, 2020

Good idea, we can probably start from that set then drill down further if need? Maybe we can trim Makefile and diff out as well (since I suspect we won't be able to see these under debug.html anyway?)

I'll also update again to latest version, 10.3.1 was just released.

@zakame zakame requested a review from jhthorsen October 19, 2020 04:53
@kraih kraih requested a review from a team October 19, 2020 13:41
jberger
jberger previously approved these changes Oct 19, 2020
View reviewed changes
Copy link
Member

@jberger jberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan of highlight.js and use it in other things.

@kraih kraih requested a review from a team October 24, 2020 11:07
@zakame
Use highlight.js instead of prettify for syntax coloring
be6dc11 
Originally based from work by @dotandimet.

Additional changes:

* highlight.js: Update to latest (10.3.1)

Install custom build limiting highlights for diff, plaintext, makefile,
bash, html/xml, sql, css, and perl only, to reduce from the normal
common pack size (100k to just 40k.)

This also adds the Mojolicious language plugin for highlight.js.

* debug.html.ep: Simplifiy highlight.js loading

Follow https://highlightjs.org/usage for this case, it seems more
reliable to let it highlight as well as determine the language to
be highlighted on its own.

* highlight-mojo-dark.css: Remake based on newer upstream dark theme

* Drop highligh-mojo-light.css

No longer needed here.

* Mojolicious.pm: Add license section for highlight.js

* Remove prettify.js

highlight.js now replaces it.  Thanks for the service, prettify.js!
This was referenced Oct 24, 2020
Closed
Merged
jhthorsen
jhthorsen approved these changes Oct 28, 2020
View reviewed changes
Copy link
Member

@jhthorsen jhthorsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the new slimmed down version of the language pack is great, so I'm in favor of this change now.

@mergify mergify bot merged commit df5ad75 into mojolicious:master Oct 28, 2020
@zakame
Copy link
Contributor Author

zakame commented Oct 28, 2020

Thanks again everyone! 🎉

@zakame zakame deleted the highlightjs branch October 28, 2020 03:01
@zakame zakame mentioned this pull request Oct 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jberger jberger jberger approved these changes

@jhthorsen jhthorsen jhthorsen approved these changes

@kraih kraih Awaiting requested review from kraih

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace prettify.js with something maintained
4 participants
@zakame @kraih @jhthorsen @jberger