asciidoctor / asciidoctor-browser-extension

:white_circle: An extension for web browsers that converts AsciiDoc files to HTML using Asciidoctor.js.
https://chrome.google.com/webstore/detail/asciidoctorjs-live-previe/iaalpfgpbocpdfblpnhhgllgbdbchmia
MIT License
216 stars 53 forks source link

Honor document attribute highlightjs-theme #63

Open ggrossetie opened 9 years ago

ggrossetie commented 9 years ago

Currently, the browser extension only includes a single theme for Highlight.js, see: https://github.com/asciidoctor/asciidoctor-browser-extension/tree/main/app/css/highlight

And the relevant code that adds the Higlight.js theme (github):

https://github.com/asciidoctor/asciidoctor-browser-extension/blob/44f78ce9a997cddd4d9ef3408473d5eabba37c16/app/js/renderer.js#L104-L108

mojavelinux commented 9 years ago

This is another feature that would benefit from splitting load and render. We gain a lot of flexibility by keeping the two phases separate. I did this in docgist so you can see an example:

https://github.com/asciidoctor/docgist/blob/master/js/docgist.js#L29

ggrossetie commented 9 years ago

I'm already using a "two step" conversion by calling load then convert method. The only "issue" I see is that I need to load the document earlier to append the correct stylesheet. Until now I was appending stylesheets and javascripts only when the page was first loaded, then the reload method was just updating the content.

mojavelinux commented 9 years ago

I realized you had already moved to the two step conversion upon further review, which is excellent.

I'd make the highlight.js loading lazy, moving it until after the first convert...because that allows us to control whether it's even enabled. I'm not sure whether we should support changing whether it can be toggled or modified after the initial rendering. I'd say it probably gets too messy and we should require a page refresh for these changes to take affect.

...how about we start with loading highlight.js lazily, only doing so if the source-highlighter attribute is "highlight.js". At that point, check the theme and load the appropriate stylesheet. Can you load the themes from cdnjs? I'm not sure we want to bundle all the themes...unless we can have bower manage them.

ggrossetie commented 9 years ago

I'd say it probably gets too messy and we should require a page refresh for these changes to take affect.

Yes I think it's no big deal to require a page refresh

Can you load the themes from cdnjs?

Yes but I also want to provide an offline mode. I don't know if it's a good or bad idea... maybe I can just use cdnjs and wait for user's request ?

mojavelinux commented 9 years ago

How about we pick a few themes to support from highlightjs and then use cdnjs if they use a theme that isn't in that list. Or perhaps the default theme is local, but any other theme we load from cdnjs. I say that because they keep adding themes to highlight.js and not all of them are good.