Use rel() more consistently in theme elements, take two #2173

karawoo · 2017-06-19T18:04:18Z

This is the continuation of #1883. Includes @baptiste's changes plus a line in NEWS.md about the change.

karawoo · 2017-06-19T18:25:57Z

Hmm, I'm surprised the Travis build passed. It looks like this change breaks the visual tests in test-theme.r. Some of the panel grid lines are a little lighter now. Here are some screenshots from vdiffr::manage_cases()

Before:

After:

hadley · 2017-06-19T19:10:42Z

R/theme-defaults.r

@@ -134,7 +134,7 @@ theme_grey <- function(base_size = 11, base_family = "") {
    panel.background =   element_rect(fill = "grey92", colour = NA),
    panel.border =       element_blank(),
    panel.grid.major =   element_line(colour = "white"),
-    panel.grid.minor =   element_line(colour = "white", size = 0.25),
+    panel.grid.minor =   element_line(colour = "white", size = rel(0.25)),


I noticed that these actually change the values of the width as the base_size (as far as I can tell) is set to 0.5 by line. I think that means all the values inside rel() should be doubled.

It also suggests that before merging this PR we should make some basic visual tests for the themes.

Ooops, should've refreshed before commenting. Glad the tests picked this up! (Although I'm not sure what went wrong on travis)

karawoo · 2017-06-19T21:17:53Z

I doubled the values inside rel() and that indeed fixed the failing visual tests on my machine. theme_linedraw() doesn't have an associated visual test, so making this change for that theme didn't affect anything testing-wise. Should I add a test for this one?

has2k1 · 2017-06-19T21:28:07Z

Tests failed on travis but not on appveyor, this is similar to #2080.

hadley · 2017-06-19T21:43:59Z

R/theme-defaults.r

@@ -204,15 +204,15 @@ theme_linedraw <- function(base_size = 11, base_family = "") {
  theme_bw(base_size = base_size, base_family = base_family) %+replace%
    theme(
      # black text and ticks on the axes
-      axis.text        = element_text(colour = "black", size = rel(0.8)),
-      axis.ticks       = element_line(colour = "black", size = 0.25),
+      axis.text        = element_text(colour = "black", size = rel(1.6)),


I think this should stay as is since it was already in a rel()?

Whoops yes, will change it back.

hadley · 2017-06-19T22:00:45Z

@karawoo do you mind doing a separate PR that ensures that there's one visual test for each theme that's bundled with ggplot2? Then we can re-run this on top of that, and be confident that we haven't changed anything accidentally.

baptiste · 2017-06-19T22:08:40Z

My original reason to look at this piece of source code and notice inconsistencies was triggered by a small plot in the margin of a Tufte-style document that looked quite bad/disproportionate. I think it may be worth setting up visual tests for a couple of different plot sizes, e.g. i) a 4x3 inches figure with 11pt font etc. for a typical publication, ii) a 24pt font, bold lines, 8x4in plot for a powerpoint, iii) a small inset-style / margin figure with 9pt font and thin lines.

(last time I looked, some themes had a few default aesthetics that I found visually suboptimal at small sizes):
https://gist.github.com/baptiste/8db48ede1eacb22cdd78093f715cb945

Small plots (e.g. in the margins of tufte-style books) tend to look heavy with the current linewidth settings, and after investigating I noticed some inconsistencies in the use of `rel()`.

…c, and linedraw

karawoo · 2017-06-20T23:45:01Z

I rebased these changes now that we've got the new test from #2174.

hadley · 2017-06-21T13:08:54Z

NEWS.md

@@ -1,5 +1,7 @@
 # ggplot2 2.2.1.9000

+* Uses `rel()` more consistently in theme defaults (@baptiste).


I think you can be a bit more explicit: use rel() to set line widths in theme defaults.

baptiste · 2017-06-21T13:21:42Z

This discussion got me thinking about an analogy to optical sizes for rendering fonts, whereby different characters undergo minute changes in specific size ranges, to improve legibility and aesthetics. I wonder if the theme system couldn't use a similar strategy to adjust line widths, font sizes, perhaps even colour and fills, depending on a global parameter (plot size is what comes to mind).

In another aspect, rel() could perhaps be extended to colours, so that one could specify a "foreground" and "background" parent colour, and the children elements would relate to that harmoniously (using either faded tones, complementarity, etc. types of palettes). That's more or less the concept used in Beamer themes.

rel() could perhaps also gain optional arguments to set an absolute min/max value, e.g. rel(0.25, min=0.1) would ensure that the linewidth would never be thinner than 0.1mm (sometimes a requirement of Editors).

lock · 2019-01-18T12:19:40Z

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

karawoo requested a review from hadley June 19, 2017 18:04

hadley reviewed Jun 19, 2017

View reviewed changes

karawoo mentioned this pull request Jun 19, 2017

Make sure all themes have visual tests #2174

Merged

baptiste and others added 3 commits June 20, 2017 16:34

use rel() more consistently for linewidths

d65e2f1

Small plots (e.g. in the margins of tufte-style books) tend to look heavy with the current linewidth settings, and after investigating I noticed some inconsistencies in the use of `rel()`.

Add line to NEWS.md about the more consistent use of rel()

8325bcf

Double values inside rel() for themes grey, bw, light, dark, classi…

f114889

…c, and linedraw

karawoo force-pushed the linewidths-rel branch from 16976a8 to f114889 Compare June 20, 2017 23:39

hadley approved these changes Jun 21, 2017

View reviewed changes

Make NEWS.md description more explicit

70770d3

karawoo merged commit 2907cf5 into tidyverse:master Jun 21, 2017

karawoo deleted the linewidths-rel branch June 21, 2017 17:12

karawoo mentioned this pull request Jun 22, 2017

Add base_line_size parameter to theme functions #2176

Closed

lock bot locked and limited conversation to collaborators Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use rel() more consistently in theme elements, take two #2173

Use rel() more consistently in theme elements, take two #2173

karawoo commented Jun 19, 2017

karawoo commented Jun 19, 2017

hadley Jun 19, 2017

hadley Jun 19, 2017

karawoo commented Jun 19, 2017

has2k1 commented Jun 19, 2017 •

edited

Loading

hadley Jun 19, 2017

karawoo Jun 19, 2017

hadley commented Jun 19, 2017

baptiste commented Jun 19, 2017 •

edited

Loading

karawoo commented Jun 20, 2017

hadley Jun 21, 2017

baptiste commented Jun 21, 2017

lock bot commented Jan 18, 2019

		@@ -1,5 +1,7 @@
		# ggplot2 2.2.1.9000

		* Uses `rel()` more consistently in theme defaults (@baptiste).

Use rel() more consistently in theme elements, take two #2173

Use rel() more consistently in theme elements, take two #2173

Conversation

karawoo commented Jun 19, 2017

karawoo commented Jun 19, 2017

hadley Jun 19, 2017

Choose a reason for hiding this comment

hadley Jun 19, 2017

Choose a reason for hiding this comment

karawoo commented Jun 19, 2017

has2k1 commented Jun 19, 2017 • edited Loading

hadley Jun 19, 2017

Choose a reason for hiding this comment

karawoo Jun 19, 2017

Choose a reason for hiding this comment

hadley commented Jun 19, 2017

baptiste commented Jun 19, 2017 • edited Loading

karawoo commented Jun 20, 2017

hadley Jun 21, 2017

Choose a reason for hiding this comment

baptiste commented Jun 21, 2017

lock bot commented Jan 18, 2019

has2k1 commented Jun 19, 2017 •

edited

Loading

baptiste commented Jun 19, 2017 •

edited

Loading