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

support code syntax highlighting #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

machow
Copy link

@machow machow commented Nov 19, 2020

addresses #5. This PR adds the name of the engine to flair's code output. This seems to allow highlightjs to do syntax highlighting. I'm not too familiar with how flair and knitr are doing things though, so happy to modify / fix :).

Test Rmarkdown Doc

---
title: "test"
output: html_document
---

```{r setup, include=FALSE}
knitr::opts_chunk$set(echo = TRUE)

library(flair)
```

```{r test}
out <- abs(-1)
a <- "a"
```

```{r}
decorate("test") %>%
  flair("-1")

```

image

image

@gadenbuie
Copy link
Contributor

gadenbuie commented Nov 19, 2020

This is a good start, thanks @machow!

One thing to watch out for is that users might modify color or other properties using flair() which will be broken by the syntax highlighting. For example:

```{r test}
out <- abs(-1)
```

```{r}
decorate("test") %>%
  flair("-1", color = "purple")
```

Should change the color of -1 to purple, but highlightjs breaks this.

image

You can see in the browser highlightjs adds a <span> around 1 inside the <span> added by flair().

<pre class="prettyprint r"><code class="hljs">out <- abs(<span style="color:purple">-<span class="hljs-number">1</span></span>)</code></pre>

I'm not sure about the best way to handle this other than to suggest that it's made possible to opt in or out of syntax highlighting via a top level argument in either flair() or decorate().

@machow
Copy link
Author

machow commented Nov 20, 2020

Hey @gadenbuie -- thanks for your quick feedback!

That does seem like a pretty rough case. I'm happy to try adding an argument to flair() or decorate() to enable the behavior in the PR (guessing should be disabled by default).

An alternative approach, that's mentioned in #27, could be allowing users to specify the <pre> classes explicitly, by adding a class.source argument to decorate (mimicking the knitr chunk opt).

WDYT? (loving using flair!)

@kbodwin
Copy link
Collaborator

kbodwin commented Jan 27, 2021

Thanks for this PR @machow, and for your input @gadenbuie.

I have always hoped to add a syntax_highlight option to decorate(), but I would prefer that the flair() choices still take preference. This is super tricky, because the syntax highlighting triggers at page render, not the knitting.

For the time being, I'd be happier with a class.source option, so it's only used by folks who are familiar enough to know they may be overriding their flair().

I'm very very interested in finding a solution to this problem, and this definitely takes us one step closer, so many thanks!

@kbodwin kbodwin force-pushed the master branch 2 times, most recently from 20076b7 to 4664b3c Compare February 10, 2021 20:26
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

Successfully merging this pull request may close these issues.

3 participants