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

resolves #21 load highlight.js from a CDN #320

Merged
merged 2 commits into from
Feb 9, 2020
Merged

resolves #21 load highlight.js from a CDN #320

merged 2 commits into from
Feb 9, 2020

Conversation

ggrossetie
Copy link
Member

@ggrossetie ggrossetie commented Jan 22, 2020 •
edited
Loading

resolves #21

It also resolves #319 even though it can be seen as a regression because the reveal.js highlight plugin contains all the languages by default.

To be backward compatible, we could use a local file assets/highlight.min.js that contains all the languages but I don't think it's a good solution (as you will rarely use all the 185 languages in a presentation).

  • add tests
  • make sure that the code is working using Node/JS

obilodeau
obilodeau reviewed Jan 23, 2020
View reviewed changes
Copy link
Member

@obilodeau obilodeau left a comment

Choose a reason for hiding this comment

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

To be backward compatible, we could use a local file assets/highlight.min.js that contains all the languages but I don't think it's a good solution (as you will rarely use all the 185 languages in a presentation).

If we merge this in 3.x, we should provide the bundled asset with all languages (no regression policy). Then in 4.0.0 we can drop the bundle and mention in the upgrade notes how to pull in the additional languages.

Or we could delay this to 4.0.0.

assets/plugin-highlight.js Outdated Show resolved Hide resolved
lib/asciidoctor-revealjs/highlightjs.rb Outdated Show resolved Hide resolved
@ggrossetie
Copy link
Member Author

Or we could delay this to 4.0.0.

No rush, I'm just experimenting for now to fully understand the impact 😉

@obilodeau obilodeau mentioned this pull request Feb 5, 2020
Merged
6 tasks
@obilodeau obilodeau added this to the 4.0.0 milestone Feb 6, 2020
@ggrossetie ggrossetie marked this pull request as ready for review February 6, 2020 07:36
@ggrossetie
Copy link
Member Author

It's based on reveal.js 3.9.2

@obilodeau
Copy link
Member

Alright, time for you to rebase.

@ggrossetie ggrossetie force-pushed the issue-21-highlightjs-cdn branch 3 times, most recently from 52d550d to 492653d Compare February 8, 2020 15:07
@ggrossetie
Copy link
Member Author

Alright, time for you to rebase.

Done 👍

obilodeau
obilodeau reviewed Feb 8, 2020
View reviewed changes
README.adoc Outdated Show resolved Hide resolved
obilodeau
obilodeau reviewed Feb 8, 2020
View reviewed changes
Copy link
Member

@obilodeau obilodeau left a comment

Choose a reason for hiding this comment

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

I've finished reviewing. It's great! I like the languages support and really grateful that keeping it up in sync shouldn't be too complicated.

Two little comments to address / discuss, then I'll merge.

@ggrossetie
Copy link
Member Author

Thanks for your review, I will take care of them later tonight or tomorrow. I've also sent a pull request on reveal.js to load highlight.js from a custom location: hakimel/reveal.js#2580
Hopefully, once it's merged, we should be able to use this feature to simplify things.

@obilodeau obilodeau merged commit 6ed7ef5 into asciidoctor:master Feb 9, 2020
@obilodeau obilodeau mentioned this pull request Feb 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obilodeau obilodeau obilodeau approved these changes

Assignees
No one assigned
Labels
🍩 improvement
Projects
None yet
Milestone
4.0.0
2 participants
@ggrossetie @obilodeau