pietroppeter / nimib

nimib 🐳 - nim 👑 driven ⛵ publishing ✍
https://pietroppeter.github.io/nimib/
MIT License
175 stars 10 forks source link

Implement nbShow and highlight.js support for code in markdown #179

Closed HugoGranstrom closed 1 year ago

HugoGranstrom commented 1 year ago

fixes #74 fixes #68

HugoGranstrom commented 1 year ago

One potential solution to the highlight.js problem is to give the user two modes:

  1. Statically highlight Nim code but do not allow highlight.js to highlight Nim code (Nim code in markdown isn't highlighted).
  2. Everything is highlighted using highlight.js.

Mode 2 could be used for files where you only have markdown for example or when you know you will have multiple lanugages. But if you will only use nbCode, mode 1 will work.

HugoGranstrom commented 1 year ago

Nevermind, found that you can mark a code block as nohighlight :tada:

pietroppeter commented 1 year ago

Will look into it and let you know. As a first quick feedback I would probably keep nim in code class (not sure why I put hljs, was it needed for the css highlighting?)

pietroppeter commented 1 year ago

Some more feedback:

pietroppeter commented 1 year ago

(Btw not even sure if this is ready for review...)

HugoGranstrom commented 1 year ago

As a first quick feedback I would probably keep nim in code class (not sure why I put hljs, was it needed for the css highlighting?)

In my minimal example adding the nim class doesn't make a difference when using nohighlighting, but adding hljs ruins it. So I could add nim back.

do we have somewhere in the documents an example where we have Nim code in a markdown context, so that we can see it in action?

Interactivity has quite a few markdown code blocks. As it stands, the two different code blocks have different styles though :/

should we make this something that we can opt out? It would require creating a new line in head partial and a new data point highlightJs in context that can be turned off. Putting next to the css feels a bit of a hack ;)

Yeah yeah yeah, we can add that when cleaning up at the end ;)

(Btw not even sure if this is ready for review...)

It's absolutely ready for review :) And changes, and a new review after them ;)

HugoGranstrom commented 1 year ago

Hold on hljs is the only thing we have in the CSS file, so nim is the one we don't need. Trying to add hljs back, seems like it doesn't break anymore locally :rofl:

pietroppeter commented 1 year ago

Interactivity has quite a few markdown code blocks. As it stands, the two different code blocks have different styles though

I have looked now and it seems it comes out fine.

I would keep both nim and hljs in the class since nim is a semantic element (and hljs needed for css). And nohighlite should be enough to making it avoid doing the work twice (have not looked if there are error in console though).

I see the tests are failing but that is because we hard coded the nbCode expected output and we changed it.

pietroppeter commented 1 year ago

nbShow looks fine!

HugoGranstrom commented 1 year ago

The only difference I have found so far is that we are highlighting common functions in the static highlighting that highlight.js is not (above is static, below is dynamic highlighting): image I don't think this difference matters that much tbh.

I would keep both nim and hljs in the class since nim is a semantic element (and hljs needed for css). And nohighlite should be enough to making it avoid doing the work twice (have not looked if there are error in console though).

Ok, will keep them then :+1:

HugoGranstrom commented 1 year ago

You can now disable highlight.js with nb.disableHighlightJs(). I'd say this is ready for a final review now.

HugoGranstrom commented 1 year ago

the highlight.min.js where is it taken from? from the past? If so I think in the past I selected a number of languages that are supported, not only nim (I see python and rust in the code, for example) I think I might have put the list in the documentation in the past.

I took common + Nim + Julia + Latex, didn't really see any other language of interest on the list. If we want to add a new one in the future, it's just a matter of generating and uploading a new js file.

we probably should mention somewhere this new highlight feature (available not only for nim but for other languages) somewhere in the docs

Good idea :+1: The question is where to put it, the readme probably?

pietroppeter commented 1 year ago

the highlight.min.js where is it taken from? from the past? If so I think in the past I selected a number of languages that are supported, not only nim (I see python and rust in the code, for example) I think I might have put the list in the documentation in the past.

I took common + Nim + Julia + Latex, didn't really see any other language of interest on the list. If we want to add a new one in the future, it's just a matter of generating and uploading a new js file.

ah good selection.

we probably should mention somewhere this new highlight feature (available not only for nim but for other languages) somewhere in the docs

Good idea 👍 The question is where to put it, the readme probably?

yes, I would go with readme, a new subsection inside "Features" about "highlighting". There is already a mention of static highlighting in the bullet list at the beginning of features, that might be reviewed. Having the list of languages (even as a list to common in highlightjs) would be useful for the user.

HugoGranstrom commented 1 year ago

yes, I would go with readme, a new subsection inside "Features" about "highlighting". There is already a mention of static highlighting in the bullet list at the beginning of features, that might be reviewed. Having the list of languages (even as a list to common in highlightjs) would be useful for the user.

I think the old text was fine, so fine indeed that I copied parts of it :P Have added the subsection and a small bit of text with a link to highlight.js's download page where the common langs are listed. Anything more you want to add?

pietroppeter commented 1 year ago

Anything more you want to add?

nope, that's good, thanks for working on this. merging!

Next step would be to release with some appropriate notes, after that we add a commit to bump version and update changelog.

HugoGranstrom commented 1 year ago

Next step would be to release with some appropriate notes, after that we add a commit to bump version and update changelog.

I'm on it!

HugoGranstrom commented 1 year ago

Have I forgotten something? For some reason it didn't allow me to create a discussion, something about there not existsing a title and description :shrug:

pietroppeter commented 1 year ago

It usually tells you that you need to create a tag to populate automatic release notes

HugoGranstrom commented 1 year ago

It usually tells you that you need to create a tag to populate automatic release notes

I'm not sure I follow, there were no problems generating the automatic release notes or creating the tag. It was just creating the discussion forum post that was the problem.

pietroppeter commented 1 year ago

Ah ok, not sure why then. Maybe you need special permissions?

HugoGranstrom commented 1 year ago

Yeah perhaps, not sure exactly what permissions I have. That should hopefully be easier to manage in nimib-org hopefully.