Panel backgrounds

[kferris10]

Moved from #78

In addition to drawing background rectangles, I amended the linetype test because the background rectangles were interfering with this test. If somebody could confirm that this makes sense, that would be great.

I've drawn the major grid lines and will get to the minor ones as soon as possible

[tdhock]

Thanks for contributing this great code and tests Kevin.

Yes it makes sense to merge this branch without implementing the panel.margin feature.

However I ran the test suite and saw the following visual regression (in both firefox and chrome)

The top facet labels are cutoff (hard to see in "Years" but the top of the F and the dot on the i are clearly missing in "Fertility rate").

Can you please add a test that fails in test-widerect.R?

And then can you please fix that bug?

[kferris10]

Thanks for catching that. It originated a couple of days ago when I was working on panel margins. I've removed it,

I'd prefer to add the test when I start working on the panel margins. Because that is what cause the issue, I think it'd be more consistent to put it there.

[cpsievert]

Maybe we could change this line to grepl("^#[0-9A-Za-z]{6, 8}$", x) ?

The value section on the help page for rgb() says:

A character vector with elements of 7 or 9 characters, "#" followed by the red, blue, green and optionally alpha values in hexadecimal (after rescaling to 0 ... 255).

cc @tdhock

[caijun]

+1

[cpsievert]

@kferris10 can't wait to merge this (looks great)!! It's a huge improvement especially for facetted plots that share an axis.

I found it a bit surprising that minor grid lines are drawn over major ones. Is that intentional (is that how ggplot2 does it)? I wrote a test to check the number of minor grid lines, but feel free to modify it if that behavior is to be expected.

[kferris10]

It looks like that's just how ggplot2 does it. Check out the major vs minor

str(ggplot_build(p2)$panel$ranges[[1]])
List of 12
 $ x.range       : num [1:2] 0.705 7.195
 $ x.labels      : chr [1:3] "2" "4" "6"
 $ x.major       : num [1:3] 0.2 0.508 0.816
 $ x.minor       : num [1:7] 0.0455 0.1995 0.3536 0.5077 0.6618 ...
 $ x.major_source: num [1:3] 2 4 6
 $ x.minor_source: num [1:7] 1 2 3 4 5 6 7
 $ y.range       : num [1:2] -0.02 2.62
 $ y.labels      : chr [1:6] "0.0" "0.5" "1.0" "1.5" ...
 $ y.major       : num [1:6] 0.00758 0.19697 0.38636 0.57576 0.76515 ...
 $ y.minor       : num [1:11] 0.00758 0.10227 0.19697 0.29167 0.38636 ...
 $ y.major_source: num [1:6] 0 0.5 1 1.5 2 2.5
 $ y.minor_source: num [1:11] 0 0.25 0.5 0.75 1 1.25 1.5 1.75 2 2.25 ...

I thought that I was drawing the major lines on top of the minor ones. Is that correct?

[tdhock]

I tried implementing a test for the unreadable strip labels but it seems complicated. How to tell if the svg <text> element is partly overlapping the top of the <svg> element?

I tried adding the code below to test-facet-trivial.R:

test_that("top strip labels are not cut off", {
  viz <-
    list(Sk=gg+facet_grid(Species ~ kingdom))
  info <- animint2HTML(viz)
  strip.rect <- remDr$executeScript('return document.getElementById("topStrip").firstChild.getBoundingClientRect()')
  svg.rect <- remDr$executeScript('return document.getElementById("Sk").getBoundingClientRect()')
})

[cpsievert]

@kferris10 I'm not positive, but from these examples, it appears ggplot2 doesn't draw the minor grid lines where the major ones are (I think prefer that behavior anyway so we don't clutter up the DOM)

qplot(Sepal.Length, Sepal.Width, data = iris) + theme( panel.grid.minor = element_line("blue"))

qplot(Sepal.Length, Sepal.Width, data = iris) + theme(panel.grid.major = element_line("red"), panel.grid.minor = element_line("blue"))

[tdhock]

I agree with Carson. The less DOM elements to create, the faster the
animint viz will be. (browsers are known to slow down after adding more
than 10,000 elements to an SVG element).

On Tue, Jun 16, 2015 at 10:42 AM, Carson notifications@github.com wrote:

@kferris10 https://github.com/kferris10 I'm not positive, but from
these examples, it appears ggplot2 doesn't draw the minor grid lines where
the major ones are (I think prefer that behavior anyway so we don't clutter
up the DOM)

qplot(Sepal.Length, Sepal.Width, data = iris) + theme( panel.grid.minor = element_line("blue"))

[image: rplot]
https://cloud.githubusercontent.com/assets/1365941/8185761/be5d9d72-140b-11e5-96b4-98fd600ca2a8.png

qplot(Sepal.Length, Sepal.Width, data = iris) + theme(panel.grid.major = element_line("red"), panel.grid.minor = element_line("blue"))

[image: rplot01]
https://cloud.githubusercontent.com/assets/1365941/8185803/084d097c-140c-11e5-80b6-cfcb55c742d9.png

—
Reply to this email directly or view it on GitHub
#84 (comment).

[kferris10]

@tdhock What about this instead? For this check, we just need to make sure that the text is pushed down more than the font size, right?

fontsizePattern <- 
  paste0("font-size: ", 
         "(?<value>.*?)", 
         "px;")

  top_text <- getNodeSet(info$html, '//g[@id="topStrip"]//text')
  top_attrs <- sapply(top_text, xmlAttrs)
  top_translate <- str_match_perl(top_attrs["transform",], translatePattern)
  top_font_size <- str_match_perl(top_attrs["style",], fontsizePattern)
  expect_true(as.numeric(top_translate[3]) > as.numeric(top_font_size[2]))

[tdhock]

in theory your argument makes sense Kevin.

However in practice on my firefox browser the default translate value of 7.25 cuts of the 11px text:

but a value of 8 (less than 11px) is sufficient for a strip label that is not cut off:

or am I missing something?

in cases like this where the test is not straightforward to write, I prefer to just not write a test (avoid false test failures in the future).

by the way the code i used was

test_that("top strip labels are not cut off", {
  miris <- iris
  miris$kingdom <- "plANtAEgjiq,!?"
  gg <- ggplot()+
    geom_point(aes(Petal.Length, Petal.Width),
               data=miris)
  viz <-
    list(Sk=gg+facet_grid(Species ~ kingdom))
  info <- animint2HTML(viz)
  strip.rect <- remDr$executeScript('return document.getElementById("topStrip").firstChild.getBoundingClientRect()')
  svg.rect <- remDr$executeScript('return document.getElementById("Sk").getBoundingClientRect()')
})

[tdhock]

tests pass on my computer with firefox. +1 go ahead and merge when you like.

[cpsievert]

Great, this also has my +1, make sure you update the version in the DESCRIPTION and the NEWS before (or after) merging @kferris10.

It'd also be awesome if you could re-build the tutorial to reflect the new theming defaults!

[kferris10]

Not a problem. Are there any other visualizations that should be rebuilt?

Also, should I just bump the version to today's date?

[cpsievert]

Just the tutorial is fine for now. And yea, use today's date.

[kferris10]

Great, this last commit should take care of that.

This means that we can also merge #68 as well, right?

[cpsievert]

Cool, go ahead and merge this.

After you merge this into master, you might have conflicts when you pull these changes into #68 (waiting to update the version and NEWS helps in this regard). Are you familiar with resolving merge conflicts? If it is a big conflict, sometime I do git mergetool which pops up a helpful GUI for resolving the conflict.

[kferris10]

Just found a bug when compiling the animint tutorial. Does it make the most sense to push the fix to this branch or to create a new one?

[tdhock]

can you please first write a test that fails for the viz with the bug?

after that I would suggest pushing the changes to a new branch and making a new PR.

[kferris10]

Already done :)

[kferris10]

I've updated the tutorial. @srvanderplas could you take a quick look at it when you get a chance? Three things in particular:

1. I set the climate visualization to remove grid lines, set a white background, and added to your explanation of the theme.
2. For some reason, there's now a problem with scale_y_ in the density examples. I commented out this line in the geom_polygon with stat = 'density2d' contours example but left it in the scale_y_log10() and geom_contour() with stat_density2d() example so you can see what I'm talking about. Figured I'd see if you have any idea what's going on before I dig into it.
3. In the abline example, I don't see the lines generated in either your example or in the updated version. Any idea what is going on here?

I'll push to this repository once we get this stuff figured out

[tdhock]

for geom_abline there are <line> elements with NaN values.

Not sure when this bug was introduced, but it did work at some point in the past.