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

feat: toggle clean_pandoc2_highlight_tags in html_document2 #706

Merged
merged 1 commit into from
May 1, 2019

Conversation

atusy
Copy link
Collaborator

@atusy atusy commented Apr 21, 2019

I implemented a toggling option to the clean_pandoc2_highlight_tags in html_document2 (Issue #705)

The toggling is achieved by specifying highlight_cleaned in a YAML front matter, which is TRUE in default for consistency with previous versions.

If there is any better name for this option, please tell me.

Here's a reproducible example.

---
output:
 bookdown::html_document2:
   highlight: pygments
   highlight_cleaned: false
---

```{r, class.source = "numberLines lineAnchors", eval = FALSE}
head(iris)
```

@yihui yihui added this to the v0.10 milestone May 1, 2019
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.

Looks good to me. The roxygen doc needs a few more markups. I'll do that by myself after merging. Thanks!

@yihui yihui merged commit 13e25ad into rstudio:master May 1, 2019
yihui added a commit that referenced this pull request May 1, 2019
@yihui
Copy link
Member

yihui commented May 1, 2019

FYI, I renamed highlight_cleaned to clean_highlight_tags.

@atusy atusy deleted the feat/toggle-clean-pandoc2-highlight branch May 2, 2019 00:13
@RLesur
Copy link
Collaborator

RLesur commented Jun 29, 2019

Please note that with Pandoc 2.7.3, the clean_highlight_tags argument does not toggle line numbering anymore: with clean_highlight_tags: true a code chunk with class numberLines will be numbered.

I do not interpret this behavior as a bug nor a regression. It seems to me that Pandoc 2.7.3 makes the clean_highlight_tags option useless. There is no issue with that.

Refs: #733 (comment) and https://github.com/jgm/skylighting/blob/master/changelog.md#08----2019-05-27

@atusy
Copy link
Collaborator Author

atusy commented Jun 29, 2019

Thanks.

I see that only the difference caused by clean_highlight_tags: true is remaining of <div class="sourceCode"></div>.
This seems to be because <a> tags no more have the sourceLine class, and thus they won't be cleaned by clean_pandoc2_highlight_tags().

Yet, true seems to be important for better styling because Pandoc embeds following CSS to modify margins when any chunk has class numberLines (see https://gist.github.com/atusy/d6d686de8982a36010c36eda94fed1a5 for full version).

div.sourceCode { margin: 1em 0; }
pre.sourceCode { margin: 0; }

By default, pre has margin based on bootstrap.min.css

pre {margin: 0 0 10px}

If we do not have to care about clean_highlight_tags, that's better in terms of user experience.
I think we have two options for that:

  • Updating clean_pandoc2_highlight_tags() to only remove <a> not <div>. Though I don't know the importance of removing <div>
  • Updating Pandoc templates to specify appropriate margins for pre.sourceCode.

@RLesur
Copy link
Collaborator

RLesur commented Jun 30, 2019

About the two options described by @atusy

  • Updating clean_pandoc2_highlight_tags() to only remove <a> not <div>.

    IMO, modifying the markup produced by bookdown is a risky option. That could have many side effects for the users and lead to regressions which are hard to resolve. See for instance, this SO question about these divs.

  • Updating Pandoc templates to specify appropriate margins for pre.sourceCode.

    IMO, modifying CSS stylesheets is from far more flexible.

@atusy
Copy link
Collaborator Author

atusy commented Jun 30, 2019

I agree with @RLesur

@yihui
Copy link
Member

yihui commented Jul 5, 2019

Hi @atusy, I'd like to get rid of the whole clean_pandoc2_highlight_tags() thing now. This hackish function was added via cd2fb76 mainly because of one thing that I didn't like about Pandoc 2.0: https://groups.google.com/d/msg/pandoc-discuss/1bwAre8fG6E/k4vM455kCQAJ Now it seems it no longer adds random IDs to lines, so clean_pandoc2_highlight_tags() is no longer useful to me (more importantly, I really don't like this hack myself).

What do you think?

@atusy
Copy link
Collaborator Author

atusy commented Jul 5, 2019

Nice! The less hackish, the less vulnerable the bookdown package is to the changes in pandoc and skylighting. Let's get rid of it.

After getting rid of it, I will close or update PR #738.
I revert the changes in clean_pandoc2_highlight_tags() and default.html.
A point is whether to modify gitbook's template or just forget about it.
What do you think?

I don't really need it because I don't use line numbers, too.
I made the PR because my friend wanted it and it was a good chance to learn inside of rmarkdown and related packages.

yihui added a commit that referenced this pull request Jul 5, 2019
…oc2_highlight_tags()` method; this is also a much more robust fix to #733
@yihui
Copy link
Member

yihui commented Jul 5, 2019

I just removed the function, although I introduced another hack... 52b71f9#diff-0e9d8f9e55fa1ae0adee00b6ed6ba976R51

I haven't tried to understand #738 yet but your description seems to be very clear. Since your issues and PRs are always so clear, just let me know if you want write access to this repo someday, and I'll be happy to grant it to you :)

@atusy
Copy link
Collaborator Author

atusy commented Jul 6, 2019

Thanks for your commits and suggestions.
I'd be happy to have write access and continue contributing 😄
(though I guess I make PRs instead of direct commits for the most of the cases)

For #738, I closed it for once because I wanted some rethinking as commented in there.

@yihui
Copy link
Member

yihui commented Jul 7, 2019

Okay, I just gave you write access. If you are very sure about a change, just push to the master branch. I wouldn't mind it. If you want to discuss, you can always send PRs. Thank you!

@atusy
Copy link
Collaborator Author

atusy commented Jul 7, 2019

Thanks a lot! May I add myself as ctb on DESCRIPTION?

@yihui
Copy link
Member

yihui commented Jul 7, 2019

Of course.

@atusy
Copy link
Collaborator Author

atusy commented Jul 7, 2019

Thanks again. I did it 😃

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants