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

Proposed changes for flexible styling in tintPdf and tintBook #30

Merged
merged 4 commits into from Nov 22, 2018

Conversation

Projects
None yet
2 participants
@jonathan-g
Copy link
Contributor

commented Nov 21, 2018

Here are my proposed changes for adding the following capabilities to tintPdf and tintBook:

  • Allow the standard latex_engine parameter to the YAML output block to allow users to select xelatex or lualatex instead of pdflatex.
  • Allow the user to specify different LaTeX font packages instead of the default Roboto. Users can specify package names and package options in the main YAML block under the optional latexfonts parameter.
  • Allow the user to change the link color from the default 0.2,0.2,0.8 using the optional linkcolor YAML parameter.
  • Allow advanced users and package authors to specify drop-in replacement Pandoc LaTeX templates. This will allow people to enhance tint by writing functions that provide templates, while relying on tint to do the other work of generating the markdown that gets fed to the template.

The PR looks big, with almost 400 lines of changes, but the vast majority of these are documentation (120 lines of new Roxygen comments in pdf.R, a dozen or so lines of Roxygen comments in html.R, and two new Rd files produced from these comments by Roxygen). The actual changes to R code and LaTeX templates is around 20 lines of R and 16 lines of LaTeX in each of the template files.

Right now, this is more geared toward a power user because selecting fonts requires knowing which LaTeX packages to include and which options they need, but if people have a solid LaTeX installation, they should be able to just copy and paste from the usage examples in the LaTeX font catalogue at http://www.tug.dk/FontCatalogue/

Here are some example YAML headers that can be substituted into the example tintPdf.Rmd file:

To use EB Garamond for the main font, with matching maths and Nimbus Mono Narrow for the typewriter font:

---
title: "Tint Is Not Tufte"
subtitle: "An implementation in R Markdown"
author: "JJ Allaire, Yihui Xie, Dirk Eddelbuettel"
date: "`r Sys.Date()`"
latexfonts: 
  - package: newtxmath
    options: 
      - cmintegrals
      - cmbraces
  - package: ebgaramond-maths
  - package: nimbusmononarrow
output: tint::tintPdf
bibliography: skeleton.bib
link-citations: yes
---

To use Lato sans-serif for the main font with Fira Mono for the typewriter font, change the link color, and use luaLaTeX for the engine:

---
title: "Tint Is Not Tufte"
subtitle: "An implementation in R Markdown"
author: "JJ Allaire, Yihui Xie, Dirk Eddelbuettel"
date: "`r Sys.Date()`"
latexfonts: 
  - package: lato
    options: default
  - package: FiraMono
linkcolor: "0.3,0.3,0.6"
output:
  tint::tintPdf:
    latex_engine: lualatex
bibliography: skeleton.bib
link-citations: yes
---

To substitute a different Pandoc LaTeX template, you would use the template parameter in the YAML output block. This is a character string, which can be either a path to the template file or a string that parses to a function call that will return the path to the template file:

output:
  tint:tintPdf:
    template: "~/my-custom-template.tex"

or

output:
  tint:tintPdf:
    template: "system.file('rmarkdown', 'templates', 'my-style', 'resources', 'my-style-template.tex', package = 'myTintEnhacement')"

Longer term, it would be good to think about how to make these options more user-friendly for people who aren't used to dealing with LaTeX.

I've tested this out and it passes R CMD check --as-cran on Windows and Ubuntu with no errors and no warnings, but some minor notes (one for the high version component .9000 which I stuck in DESCRIPTION to follow Hadley's convention for development versions and one for a spurious URL under Ubuntu due to a 404 from http://edwardtufte.com/tufte/books_be (the URL is good, but the server is probably configured to block automated requests)

jonathan-g added some commits Nov 21, 2018

Updated the package to allow users to override the default fonts, lin…
…k color, and LaTeX engine.

Users can also override the default Pandoc LaTeX template for PDF output.
Update travis dist to xenial
Trusty has an out-of-date pandoc (1.12.2.1) that does not work with the current rmarkdown/knitr (which requires >= 1.12.3), so update the travis.yml to use the xenial distribution (officially available on travis as of 2018-11-08).
@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 21, 2018

This is pretty long but quickly:

  • Incremental versions are .1 -- make it 0.1.0.1, 0.1.0.2 if needed and so on. I don't care what (bad) defaults devtools may use. Don't make an extra commit for it but if you're at it for another please change.
  • The Tufte URL has to go if it fails. CRAN makes no exceptions here either. Links must work.
  • The Travis fail needs to get fixed but I see you are on it.
    More later.
@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

The changes look good in general. I will test later. This is a lovely feature extension.

Stylistic nagging:

  • ChangeLog: Please try to use the more concise style I have been employed.
  • DESCRIPTION: I am not a fan of Authors@R as I am creator/maintainer here I would like to ask you to revert that to a standard free format list.
  • I am not sure I am a fan off the added help files. I might have preferred new/expanded sections in the existing ones. It scatters documentation over many files. But let's leave it as is for now.
  • As a general rule, when I contribute to other packages I try not to turn the existing style upside down.

Feel free to harp back if you feel very strongly about any of these points. But as we have the means to code review here, I feel somewhat strongly that it makes packages better if we actually do it.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

One other thing: Best way to showcase the use of the new fonts? Screenshots on the README.md? Extra (shorter) vignettes?

@jonathan-g

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

  • Will change the version on my next commit.
  • Fixed Travis: the issue was trusty has Pandoc 1.12.2.1 and current rmarkdown requires >=1.12.3 (https://cran.r-project.org/web/packages/rmarkdown/index.html). I changed travis.yml to use dist: xenial, but an alternative would be to apt-get install a more up-to-date backport under trusty.
  • I will address the URL check problem.
@jonathan-g

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

I will make the requested style changes in ChangeLog and DESCRIPTION. You're the creator/maintainer and it's right for me to follow your style.

I'll work on making short vignettes to illustrate the new font options.

@jonathan-g

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

It should be easy to move the extra help material from separate files to a new section under tintPdf. Very little work, and if that's the way you like to do things, it's easy enough for me to make the change.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

Awesome, and sorry for the waterfall. Just got home. Am surprised that Travis bonked now -- I guess I didn't trigger a run since rmarkdown changed to requiring a newer version of pandoc so well done.

R-release checks fine; R-devel just got a local hangup here as my R-devel installation did not have ggplot2 and its tail of depends. Installing those now. But got cleanly to vignettes so did not bite me on the missing URL. win-builder/CRAN would though so we may have to refer it indirectly.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

I am on the fence on the where the help should go. No need to rush. Sometimes I like having it in one file. Sometimes I like it being more explicit.

How about if we mix and match? Maybe consider sticking all Rd content into one, but be explicit about the options, choices, effects, ... in another (possibly static with pre-rendered screenshots) vignette?

It's very powerful stuff and folks will want to see the effects so an added vignette would be nice. We can cook that up the next few days.

R-devel checked cleanly here too. Nice work!

@jonathan-g

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

I like your thoughts about the vignettes.

The mix and match for the help sounds good. I won't rush on that, but it's easy enough to move the text around.

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

I can also merge now and did the itty pitty clean up on my side. We'll have follow-up commits anway, eg another PR for another (short) vignette?

@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

Argument in favour of keeping new manual pages (and to maybe look into the section or grouping the others have):

image

We already had a bunch. Some of them are short but I don't think it matters greatly. Can leave as is.

I'll just merge, do some tinkering here and either commit back later or early tomorrow.

@eddelbuettel eddelbuettel merged commit a8a1daf into eddelbuettel:master Nov 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eddelbuettel

This comment has been minimized.

Copy link
Owner

commented Nov 22, 2018

And I made you a collaborator so you have push access too. Still best to mutually review so I'll make my touchups a PR, ok?

@jonathan-g

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

Sounds good. It's very nice to collaborate with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.