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.

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

Handle multiple classes on code blocks #3

Closed
metasoarous opened this issue Jan 13, 2019 · 10 comments
Closed

Handle multiple classes on code blocks #3

metasoarous opened this issue Jan 13, 2019 · 10 comments

Comments

@metasoarous
Copy link

First off, thanks for this library!

I see that it's possible to specify a single class for a code block by append it to the tick marks in the markdown, as is possible with GitHub markdown in order to specify language type for formatting. However, while GitHub is fine with you specifying multiple such classes, markdown-to-hiccup treats the second class name as though it's part of the content of the code block. Example:

```edn vega-lite
{:data {:values [{:a 1 :b 3} {:a 4 :b 8} {:a 7 :b 5}]}
 :mark "point"
 :encoding {:x {:field "a"} :y {:field "b"}}}
```

in markdown-to-hiccup =>

vega
{:data {:values [{:a 1 :b 3} {:a 4 :b 8} {:a 7 :b 5}]}
 :mark "point"
 :encoding {:x {:field "a"} :y {:field "b"}}}

While GitHub gives us this:

{:data {:values [{:a 1 :b 3} {:a 4 :b 8} {:a 7 :b 5}]}
 :mark "point"
 :encoding {:x {:field "a"} :y {:field "b"}}}

It would be extremely valuable for me as I'm adding some functionality to https://github.com/metasoarous/oz which would make it possible to embed vega/vega-lite visualizations in markdown documents using this syntax. The reason two classes is important is that visualizations might either be json or edn, but I obviously don't want to assume every such chunk of code should be rendered as vega-lite, so I need another class to accomplish that. I could have separate classes like json-vega-lite & edn-vega-lite, but that is somewhat annoying, and won't lead to good rendering as regular markdown (say on GH).

Are you open to markdown-to-hiccup matching the GitHub behavior with respect to this issue?

Thanks in advance!

@mpcarolin
Copy link
Owner

Hi there!

This library does a few small things: it links together markdown-clj, which takes markdown and outputs a standard HTML string, and hickory, which then takes that string and converts it into hiccup. I do some decoding/encoding work and other small manipulations of the resulting hiccup, but I don't have that much control, at least until I find the time to write a direct markdown -> hiccup converter. :)

I think I've narrowed down the culprit to markdown-clj. When I tried entering your code block into that library's live demo site here, the result was exactly what you encountered.

I know the author @yogthos is open to both user suggestions and pull requests (see yogthos/markdown-clj#73). I suggest heading there first.

But if you still think markdown-to-hiccup is more responsible for this type of change, let me know!

@yogthos
Copy link

yogthos commented Jan 15, 2019

Hi,

As a note, commonmark-hiccup might be a better fit here because it parses markdown into Hiccup directly. This avoids the step of generating the HTML string and parsing it using hickory.

@mpcarolin
Copy link
Owner

Thanks for the link! You may have deprecated my library in one comment haha. I will check it out and if it covers all the bases this library does then I will post a note in the readme directing people to commonmark-hiccup.

@yogthos
Copy link

yogthos commented Jan 15, 2019

It doesn't look like commonmark-hiccup is actively maintained though, so might make sense to take it over. :)

@mpcarolin
Copy link
Owner

I may try just that 👍

@metasoarous
Copy link
Author

Great! Thanks for the feedback!

@mpcarolin
Copy link
Owner

@metasoarous, I saw your PR to clj-markdown. I will update this library’s version of clj-markdown later today to include those changes.

@metasoarous
Copy link
Author

@mpcarolin Wonderful! Thank you! I don't see a release on Clojars yet, but I noticed you're using deps.edn, which should allow you to specify a git dependency pointing to the merge commit in the interim, if you're so inclined.

Thanks again!

@mpcarolin
Copy link
Owner

@metasoarous

Okay just deployed it as version 6.1. Thank you for your patience!

@metasoarous
Copy link
Author

Amazing! Thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants