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 more highlighting support for HTML output #1941

Merged
merged 62 commits into from
Jan 14, 2022
Merged

Add more highlighting support for HTML output #1941

merged 62 commits into from
Jan 14, 2022

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Nov 4, 2020

This PR aim to close #1881 by adding an option support for downlit highlighting using a new argument in the format, named highlight_downlit after the one chose in distill

To do

Support in html_document

  • post processing step on the resulting html document
  • All document based on html_document could benefit from this by activating the feature.
  • Test it is working in gitbook)
  • Choose the default highlight theme (see below)

Support in markdown output

  • downlit::downlit_md_path() could be used, but only support Chroma for now
  • Also it adds span tags for highlighting but we want only autolink ? 🤔 Pandoc may remove them depending on the extensions, so pre process ?
  • Support in github_document
  • Support in md_document

Edit: Won't add it here. See #1881 (comment)

About highlighting

  • By default, rmarkdown uses highlightjs with the default them in html_document
  • If downlit is enabled, default (highlight = "default") will be a pandoc style and not highlightjs.
  • downlit requires to add a specific css code so that the code has links but the highligting stay the one expected from pandoc the highlight.

@cderv
Copy link
Collaborator Author

cderv commented Nov 5, 2020

default syntax highlighting when downlit is used is now a11y.theme like in distill and bookdown::bs4_book()

R/html_document.R Outdated Show resolved Hide resolved
R/html_document.R Show resolved Hide resolved
R/pandoc.R Outdated Show resolved Hide resolved
inst/rmarkdown/highlight/a11y.theme Outdated Show resolved Hide resolved
R/html_document.R Outdated Show resolved Hide resolved
R/html_document.R Show resolved Hide resolved
R/pandoc.R Show resolved Hide resolved
tests/testthat/test-pandoc.R Outdated Show resolved Hide resolved
@cderv
Copy link
Collaborator Author

cderv commented Nov 5, 2020

Just a note that this is also working with gitbook as highlight_downlit = TRUE will be correctly pass to html_document

@cderv
Copy link
Collaborator Author

cderv commented Nov 5, 2020

All this highlighting stuff started by downlit support in rmarkdown made me think. I found it was not clear enough in the current code, so I rewrote the whole function on the base of highlighting engine + syntax highlight theme. (Could still be clarified I think)

  • Same logic as before - I added some tests to check that
  • Rewrote the if-else code to be better align with engine + theme
  • Added support for *.theme file passed in the highlight = arguement (no more match.args on pandoc highgligth style name)
  • Added support for highlight = "a11y" and highlight = "rstudio", based on distill .theme file
  • This allow also to be more aligned with recent change in distill (we may be able to refactor as distill uses html_document)

I did this only for the html format, but this could be apply also to other format (not downlit, but the use of .theme file).

I still need to update the documentation and look at the other reviews.

@cderv
Copy link
Collaborator Author

cderv commented Jan 5, 2021

One thing I missed to handle here is that there are some cases where we want to use downlit to produce highlighting for Chroma CSS. This is the case for blogdown::html_page(). The current support in rmarkdown would allow to have downlit applies but will not produce the correct HTML tags for Chroma as it currently uses classes_pandoc() only and can only be activated if Pandoc highlighting style is used.

blogdown::html_page() would require highlight = NULL and highlight_downlit = TRUE with downlit::classes_chroma()

Either we should support both in here (using an option or another switch), or this is specific to blogdown and should be handle there directly.

@cderv cderv added the next to consider for next release label May 18, 2021
@cderv cderv marked this pull request as ready for review January 13, 2022 16:18
@cderv cderv requested a review from yihui January 13, 2022 16:59
@cderv
Copy link
Collaborator Author

cderv commented Jan 13, 2022

@yihui as we are focusing on rmarkdown currently, I have updated the PR and checked everything is ok.
The above thread seems overwhelming but at the change is quite small thanks to the previous reviews and feedbacks I had last year. Maybe no need to go through all the thread.

To sum-up, this PR reworks and document the highlighting logic. It has several aims:

This should work fine for html_documen(). However, there are other challenge to make it work in blogdown (when not using Pandoc as markdown renderer leading to Chroma for highlighting) and bookdown (because of the way xml2 is rewriting the HTML conflicting with bookdown processing).

Thanks.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Okay, I didn't review this PR carefully or read the conversations above. Please feel free to merge if/whenever you feel it's ready. Thanks!

cderv added 3 commits January 14, 2022 10:52
default needs to be change to a pandoc theme instead when `highlight_downlit` is TRUE
@cderv cderv changed the title add optional downlit support and clarify highlighting process Add more highlighting support for HTML output Jan 14, 2022
@cderv cderv merged commit 4c31109 into main Jan 14, 2022
@cderv cderv deleted the downlit-support branch January 14, 2022 10:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
next to consider for next release
Projects
Archived in project
4 participants