asciidoctor-browser-extension icon indicating copy to clipboard operation
asciidoctor-browser-extension copied to clipboard

Honor document attribute highlightjs-theme

Open ggrossetie opened this issue 9 years ago • 5 comments

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

ggrossetie avatar Aug 25 '14 17:08 ggrossetie

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

mojavelinux avatar Aug 26 '14 08:08 mojavelinux

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.

ggrossetie avatar Sep 01 '14 13:09 ggrossetie

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.

mojavelinux avatar Sep 01 '14 20:09 mojavelinux

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 ?

ggrossetie avatar Sep 02 '14 19:09 ggrossetie

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.

mojavelinux avatar Sep 05 '14 07:09 mojavelinux