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

update highlightjs to support more languages #185

Merged

Conversation

eksperimental
Copy link
Contributor

highlight.js has been updated

Pending Improvements:

  1. Elixir markup have some issues, It needs to be reported to the maintainers. I have used this test file
  2. We are lacking Iex support, if anybody wants to tackle down on it. There is already support for it in pygments, I'm not sure if that helps when porting it to highlight.js
  3. Support for markdown is very poor and there is not much highlighting
  4. I have added to the header of the file html files the languages supported, and their alias, just because I didn't know where to put it in the ex_doc documentation, so if anybody knows a good place, we can add it, and provide examples of how to include code from other languages.
    // Languages supported and their aliases
    bash, sh, zsh,
    coffeescript, coffee, cson, iced,
    css,
    diff, patch,
    elixir, iex,
    erlang, erl,
    erlang-repl,
    http,
    javascript, js,
    json,
    markdown, md, mkdown, mkd,
    sql,
    xml, html, xhtml, rss, atom, xsl, plist,

@eksperimental
Copy link
Contributor Author

Pending: I still need to add test for set_codeblock_language/1

@josevalim
Copy link
Member

Thank you but let's not support other languages yet. :) People complained a lot about invalid markup the last time. If people want custom markup, they can inject their own highlight.js versoin.

@eksperimental
Copy link
Contributor Author

Sorry about the last bug Jose, I understand your concerns and how bad code might have been looking. But I think with the new changes, not including these languages doesn't solve anything.
The root mistake here was to let the javascript library detect the languages by itself. In this PR, if the codeblock language is missing, the class is set in html as "elixir". Alternatively, if they language specified is not a supported one, highlight.js doesn't try to autodetect it, meaning, that it won't be formatted at all.

You can see the results in this test page, an the different test cases (no highlight, no language, language not supported):
http://eksperimental.github.io/elixir/highlight_sample/FooBar2.html
and the .ex file
https://github.com/eksperimental/eksperimental.github.io/blob/master/highlight_sample/foobar.ex

Please let me know what you think, so I can make the necessary changes if you still consider removing language support is the best option.
Thanks

@josevalim
Copy link
Member

Ok. I see, thanks! I think we should move this code somewhere else so we can test it easily. Also, I don't think we need to specially handle erlang or html/xml. :)

@eksperimental
Copy link
Contributor Author

I thought would be common, but .. it wouldn't be common to have erlang code in elixir (sorry maybe it is a silly question),
or in projects developed with phoenix to include html code in the source, or maybe dealing with templating functions..

I was thinking of putting that code somewhere, but I have no idea where.
Should it go into a fixture, or where do you think it is the best place.

@josevalim
Copy link
Member

I think we could include it the ExDoc.Markdown.to_html function directly. What do you think?

@eksperimental
Copy link
Contributor Author

Sorry, i think i misundertood completely your previous two messages. that's why the random answer.
I moved it into ExDoc.Markdown.to_html,
now i see what you meant by removing erlang and xhtml
I think we are good to go.

@josevalim
Copy link
Member

Thank you! We just need a test now! ❤️ 💚 💙 💛 💜

<script>hljs.initHighlightingOnLoad();</script>
<script type="text/javascript" charset="utf-8">
hljs.initHighlightingOnLoad();
// languages supported: 'bash', 'coffeescript', 'css', 'diff', 'elixir', 'erlang', 'erlang-repl', 'http', 'javascript', 'json', 'markdown', 'sql', 'xml'
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to include the languages once again here and down below since we include them in the highlight.js script!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because i don't know where to put the information about the aliases.
We should add it to the documentation, or README.md, ... I 'm clueless where to add it to

  /* languages supported and their aliases
    bash, sh, zsh,
    coffeescript, coffee, cson, iced,
    css,
    diff, patch,
    elixir, iex,
    erlang, erl,
    erlang-repl,
    http,
    javascript, js,
    json,
    markdown, md, mkdown, mkd,
    sql,
    xml, html, xhtml, rss, atom, xsl, plist,
  */

Copy link
Member

Choose a reason for hiding this comment

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

We could add to the README. Actually, a section about syntax highlighting and mentioning that we use highlight.js would be really welcome!

@eksperimental
Copy link
Contributor Author

yes. please test as much as possible
update docs are here: http://eksperimental.github.io/elixir/doc/elixir/

@tonini @whatyouhide @knewter

@whatyouhide
Copy link
Member

Everything looks nice at first sight. I got all excited thinking this was a fix to highlightjs/highlight.js#730 or highlightjs/highlight.js#707 😞 Boy we need those fixes! :)

@josevalim
Copy link
Member

Oops, sorry. This conflicted with the other pull request from you I just merged. Please rebase this one and I will get it in right away!

@eksperimental
Copy link
Contributor Author

there you go

josevalim added a commit that referenced this pull request Feb 4, 2015
update highlightjs to support more languages
@josevalim josevalim merged commit c4463b3 into elixir-lang:master Feb 4, 2015
@josevalim
Copy link
Member

❤️ 💚 💙 💛 💜

@eksperimental
Copy link
Contributor Author

Thank you José!

@eksperimental eksperimental deleted the highlightjs_headers2++ branch February 7, 2015 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants