ggplot2-book icon indicating copy to clipboard operation
ggplot2-book copied to clipboard

A few comments on existing themes chapter

Open clauswilke opened this issue 6 years ago • 8 comments

I have a few minor comments regarding the chapter on themes:

  1. The number of theme elements keeps growing, and it's unclear that adding an exhaustive description of all to the book makes sense. Maybe in the beginning of Section 15.4 mention that only the most commonly used elements are described here?

  2. I am now of the opinion that updating the default theme via theme_set() or theme_update() is almost never a good idea, except in the case when an extension package wants to define new theme elements. Maybe add a sentence cautioning users to get into the habit of adding themes explicitly to their plots rather than changing the default?

clauswilke avatar Dec 17 '19 18:12 clauswilke

Would you have any interest in preparing an update to the chapter? No problems, if not, but I wanted to at least give you the opportunity (obviously I make some money from the book (although not a lot) so in return, I’d give a donation to a charity of your choice).

hadley avatar Dec 17 '19 18:12 hadley

I'm happy to tackle this. Honestly it's not lot of work. Mostly the chapter is in a good state. Issue #171 requires more work, but I'm happy to tackle that also.

clauswilke avatar Dec 17 '19 18:12 clauswilke

I can make a draft for #171 as part of my writing on extensions, and you can comment there, if that is of interest?

thomasp85 avatar Dec 17 '19 18:12 thomasp85

@thomasp85 Sure, that works also.

clauswilke avatar Dec 17 '19 19:12 clauswilke

@clauswilke can you elaborate on 2) do you mean this in the most general sense, i.e. users should not set a preferred default theme in the beginning of a Rmarkdown script or in their .Rprofile? Or packages should never alter the default theme?

thomasp85 avatar Dec 17 '19 19:12 thomasp85

For packages it's definitely a bad idea to alter the default theme, they should never do that, except to add new theme elements for special features they provide.

For scripts and interactive use, people can do whatever works for them. However, I'd say it's still preferable to not alter the default theme, because there could be weird or difficult-to-debug issues if they also use an extension package that provides new theme elements. In this case, whether things work as expected may depend on whether the extension package is loaded before or after the default theme is modified, and how the default theme is modified (theme_set() vs theme_update() vs theme_replace()). So in general, I would just always recommend setting the theme directly in the plot. The book can word this appropriately as a recommendation and good practice rather than an imperative.

As an aside, after having made several hundred publication-quality figures with ggplot2, I am convinced that there isn't a single universal theme that always works. Thus, getting into the habit of adding the appropriate theme to each plot is good practice, and it adds only one line of code.

clauswilke avatar Dec 17 '19 20:12 clauswilke

hmm... I would say that this is an unintended side-effect of the new theme-inheritance code. We should work to mitigate this soon (probably not for this release though)

I think it is undesirable that the inheritance tree is linked to the default theme for exactly this reason. While I agree that a single theme is never good for every plot, changing the default look is not something I think should have negative side effects, and I'm reluctant to recommend against it

thomasp85 avatar Dec 18 '19 08:12 thomasp85

Fair enough. I think there's a simple solution: We need a second-level default that can be used as fallback if something is missing in the regular default theme. The current code uses the default theme exclusively as fallback, and hence de-facto imposes stricter rules on what can be done with the default theme. I'll open an issue and we can discuss API. Implementation would be straightforward and could be done in a day or two, so if we agree on the approach and API I'd rather do it now than wait for the next release.

clauswilke avatar Dec 18 '19 14:12 clauswilke