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

Add code highlighting #31

Merged
merged 15 commits into from
Nov 23, 2018
Merged

Add code highlighting #31

merged 15 commits into from
Nov 23, 2018

Conversation

mrvdb
Copy link
Collaborator

@mrvdb mrvdb commented Nov 20, 2018

This is a first stab at having a configurable code highlighting option,
similar to the MathJax rendering option. This change makes a checkbox
in the settings for code highlighting using the highlightjs.org
library.
What works: code highlighting in multi-user env is like I would
expect. single and anon(?) needs work

Things to resolve/consider:

  • does the .IsCode test for code highlighting need to stay? At least
    this and that should use the same version of the highlight.js lib.
  • can the common templating part be 'included' somehow?
  • the anon vs single-user vs multi-user code is not completely
    clear (to me)
  • bring js to local instead of cloudfare cdn (perhaps combine with
    MathJax)

This is a first stab at having a configurable code highlighting option,
similar to the MathJax rendering option. This change makes a checkbox
in the settings for code highlighting using the highlightjs.org
library.
What works: code highlighting in multi-user env is like I would
expect. single and anon(?) needs work

Things to resolve/consider:
- does the .IsCode test for code highlighting need to stay? At least
this and that should use the same version of the highlight.js lib.
- can the common templating part be 'included' somehow?
- the anon vs single-user vs multi-user code is not completely
clear (to me)
- bring js to local instead of cloudfare cdn (perhaps combine with
MathJax)
@thebaer
Copy link
Member

thebaer commented Nov 20, 2018

Thanks for contributing! To answer your questions:

  • does the .IsCode test for code highlighting need to stay? At least
    this and that should use the same version of the highlight.js lib.

That code type for posts is tangled up in more than just the template -- it's also used in the Chrome extension. It might be good to remove eventually, but for now I'd say we should just use the same highlight.js lib for it.

  • can the common templating part be 'included' somehow?

You could include the common parts in a similar way to how the templates/include/posts.tmpl template is included. Basically, create a new template file and make sure it's rendered by editing initTemplate() in templates.go.

  • the anon vs single-user vs multi-user code is not completely clear (to me)

You're right, single-user functionality (especially around posts) is quite finished. But see below for possible solution.

  • bring js to local instead of cloudfare cdn (perhaps combine with MathJax)

We can tackle this in a future pull request if you want.


From a product standpoint, this is a great addition. But I think it should be more of a behind-the-scenes feature and doesn't need to be an explicit user option (the fewer, the better).

MathJax is an explicit option because it potentially messes with the default Markdown rendering. I don't think that's necessary here because most people will probably want syntax highlighting in code blocks, especially if they explicitly choose a language by doing something like: ```python

So I'd suggest that instead of making it a server-side setting, syntax highlighting should simply work when people add code to their posts. To do this, the JS should check for the existence of code blocks (as you're doing with the var x = document.querySelectorAll("code[class^='language-']"); line), and if (x.length > 0) then we should dynamically load the highlight.js library (in a similar way to how webfont.js is loaded) and apply syntax highlighting.

This keeps pages as light as possible / free of unnecessary JS in the absence of code blocks, while still supporting syntax highlighting. It also reduces friction by keeping the number of options for users as low as possible.

@mrvdb
Copy link
Collaborator Author

mrvdb commented Nov 20, 2018

  • i'll sync up the highlightjs versions, is 8.4 required? if so I'll adapt.
  • i was hoping for some construct like {{include "common-highlight template"}} instead of having to adjust the template initialisation code specifically. Probably postpone this bit for later and just live with the duplicate template changes for now I guess.
  • i made it a server setting for now to have a bit more control; agreed conceptually though. I'll see iif I can make it implicit and if hightlightjs can be massaged not to be too eager.

@thebaer
Copy link
Member

thebaer commented Nov 20, 2018

  • I'm not tied to any specific version -- the latest should be fine
  • You can create multiple templates inside a single file, so you could always do something like adding a generically-named template in templates/include/ and then inside that file:
{{define "common-highlight"}}...{{end}}
{{define "future-template"}}...{{end}}

But yeah, we can always do that later.

  • Sounds good.

* master:
  Don't automatically include "v"  from git
  Get versioninfo from the git repository
It now loads unconditionally and highlights all code blocks.

TODO: optimize to not load when there are no blocks
Might as well do this right away, we're here anyway.
We load it always, no need for a user config UI
@mrvdb
Copy link
Collaborator Author

mrvdb commented Nov 20, 2018

Couple of remarks:

  • loading is still unconditional, could not get a conditional js load to work properly on load event. I'm probably missing some subtlety there See add06ee

  • name of the common template is a bit vague, any suggestions for a better name?

  • isCode part loads css, highlight part too, there's probably a conflict there, how to test?

@mrvdb
Copy link
Collaborator Author

mrvdb commented Nov 22, 2018

This could now be merged from my pov.

* upstream/master:
  Work as a standalone server, including TLS
  Include About/Privacy page content in page description
  Show instance stats on About page
  Change default database name to writefreely
  Use and validate database type before connecting
  Mention Contributing Guide in README
  Add AUTHORS.md
  Fix About page link in Admin dash
  Include version in archives made by `make release`
  Remove keys.sh from make release
  Add make release
@thebaer
Copy link
Member

thebaer commented Nov 22, 2018

Nice! Great work here. To answer the remaining questions:

  • Not sure of a better name off the top of my head -- I'll need to think of one
  • To test the IsCode part you'd create a post where the font parameter is code -- this would need to be done through the API, as the web client doesn't support it.

I'll review / test everything out a little later today or tomorrow.

templates.go Outdated
filepath.Join(templatesDir, "base.tmpl"),
))
} else {
templates[name] = template.Must(template.New("").Funcs(funcMap).ParseFiles(
filepath.Join(templatesDir, name+".tmpl"),
filepath.Join(templatesDir, "include", "footer.tmpl"),
filepath.Join(templatesDir, "include", "render.tmpl"),
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 want this included in every templated page, but we do need it if name == "collection" || name == "collection-tags" || name == "collection-post" || name == "post"

I think the best way to do this will be passing a []string to a single ParseFiles() call, where that array is constructed based on which template is being initialized. I'll make this change when I review this a little later.

@thebaer
Copy link
Member

thebaer commented Nov 23, 2018

Okay, made those changes, and everything looks good! Would love to get support for more languages, but we can always do that later. Merging now.

@thebaer thebaer merged commit cf78438 into writefreely:master Nov 23, 2018
@mrvdb
Copy link
Collaborator Author

mrvdb commented Nov 23, 2018

Cool, thanks.

Having more languages is indeed something I would like to have too. Perhaps after the move to local js change?

@thebaer
Copy link
Member

thebaer commented Nov 23, 2018

Yep, that'll work. Was thinking we could dynamically load in libraries for individual languages too, since there are so many available.

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

Successfully merging this pull request may close these issues.

None yet

2 participants