From c28aed1a50fc52af0bbdac63b33904e5063642af Mon Sep 17 00:00:00 2001 From: timelyportfolio Date: Wed, 4 May 2016 13:31:10 -0500 Subject: [PATCH 1/6] begin to implement an approach to temporarily and partially deal with https://github.com/ropensci/plotly/issues/510 --- R/ggplotly.R | 68 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/R/ggplotly.R b/R/ggplotly.R index 14feafdf25..1b970e0e68 100644 --- a/R/ggplotly.R +++ b/R/ggplotly.R @@ -335,6 +335,9 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A } # type of unit conversion type <- if (xy == "x") "height" else "width" + # get axis title + axisTitleText <- sc$name %||% p$labels[[xy]] %||% "" + if (is_blank(axisTitle)) axisTitleText <- "" # https://plot.ly/r/reference/#layout-xaxis axisObj <- list( type = "linear", @@ -360,7 +363,10 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A gridcolor = toRGB(panelGrid$colour), gridwidth = unitConvert(panelGrid, "pixels", type), zeroline = FALSE, - anchor = anchor + anchor = anchor, + # if not facets then use Plotly axis titling mechanism + # see https://github.com/ropensci/plotly/issues/510 + title = axisTitleText ) # convert dates to milliseconds (86400000 = 24 * 60 * 60 * 1000) # this way both dates/datetimes are on same scale @@ -380,18 +386,6 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A # do some stuff that should be done once for the entire plot if (i == 1) { - # add space for exterior facet strips in `layout.margin` - if (has_facet(p)) { - stripSize <- unitConvert(stripText, "pixels", type) - if (xy == "x") { - gglayout$margin$t <- gglayout$margin$t + stripSize - } - if (xy == "y" && inherits(p$facet, "grid")) { - gglayout$margin$r <- gglayout$margin$r + stripSize - } - } - axisTitleText <- sc$name %||% p$labels[[xy]] %||% "" - if (is_blank(axisTitle)) axisTitleText <- "" axisTickText <- axisObj$ticktext[which.max(nchar(axisObj$ticktext))] side <- if (xy == "x") "b" else "l" # account for axis ticks, ticks text, and titles in plot margins @@ -399,8 +393,7 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A gglayout$margin[[side]] <- gglayout$margin[[side]] + axisObj$ticklen + bbox(axisTickText, axisObj$tickangle, axisObj$tickfont$size)[[type]] + bbox(axisTitleText, axisTitle$angle, unitConvert(axisTitle, "pixels", type))[[type]] - # draw axis titles as annotations - # (plotly.js axis titles aren't smart enough to dodge ticks & text) + if (nchar(axisTitleText) > 0) { axisTextSize <- unitConvert(axisText, "npc", type) axisTitleSize <- unitConvert(axisTitle, "npc", type) @@ -409,21 +402,42 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A bbox(axisTickText, axisText$angle, axisTextSize)[[type]] - bbox(axisTitleText, axisTitle$angle, axisTitleSize)[[type]] / 2 - unitConvert(theme$axis.ticks.length, "npc", type)) - # npc is on a 0-1 scale of the _entire_ device, - # but these units _should_ be wrt to the plotting region - # multiplying the offset by 2 seems to work, but this is a terrible hack - offset <- 1.75 * offset - x <- if (xy == "x") 0.5 else offset - y <- if (xy == "x") offset else 0.5 - gglayout$annotations <- c( - gglayout$annotations, - make_label( - faced(axisTitleText, axisTitle$face), x, y, el = axisTitle, - xanchor = "center", yanchor = "middle" + } + + # add space for exterior facet strips in `layout.margin` + if (has_facet(p)) { + stripSize <- unitConvert(stripText, "pixels", type) + if (xy == "x") { + gglayout$margin$t <- gglayout$margin$t + stripSize + } + if (xy == "y" && inherits(p$facet, "grid")) { + gglayout$margin$r <- gglayout$margin$r + stripSize + } + # draw axis titles as annotations + # (plotly.js axis titles aren't smart enough to dodge ticks & text) + if (nchar(axisTitleText) > 0) { + # npc is on a 0-1 scale of the _entire_ device, + # but these units _should_ be wrt to the plotting region + # multiplying the offset by 2 seems to work, but this is a terrible hack + offset <- 1.75 * offset + x <- if (xy == "x") 0.5 else offset + y <- if (xy == "x") offset else 0.5 + gglayout$annotations <- c( + gglayout$annotations, + make_label( + faced(axisTitleText, axisTitle$face), x, y, el = axisTitle, + xanchor = "center", yanchor = "middle" + ) ) - ) + } } } + + if (has_facet(p)) { + # turn off plotly axis titles + # since we need special treatment for facets + gglayout[[axisName]]$title <- "" + } } # end of axis loop From baa4945e2b73a15729b043b9fe960a6380b1b24f Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Thu, 5 May 2016 12:39:52 +1000 Subject: [PATCH 2/6] change tests to reflect change in axis titles --- tests/testthat/test-cookbook-axes.R | 2 +- tests/testthat/test-ggplot-labels.R | 4 ++-- tests/testthat/test-ggplot-legend.R | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/testthat/test-cookbook-axes.R b/tests/testthat/test-cookbook-axes.R index 031fd18b6b..f7f546c189 100644 --- a/tests/testthat/test-cookbook-axes.R +++ b/tests/testthat/test-cookbook-axes.R @@ -107,7 +107,7 @@ no.x.title <- bp + test_that("coord_fixed(ratio)", { info <- expect_traces(no.x.title, 1, "no-x-title") - expect_true(length(info$layout$annotations) == 1) + expect_identical(info$xaxis$title, "") }) # Also possible to set the axis label with the scale diff --git a/tests/testthat/test-ggplot-labels.R b/tests/testthat/test-ggplot-labels.R index 2da9741382..723b93484c 100644 --- a/tests/testthat/test-ggplot-labels.R +++ b/tests/testthat/test-ggplot-labels.R @@ -13,8 +13,8 @@ test_that("ylab is translated correctly", { geom_point(aes(Petal.Width, Sepal.Width)) + ylab("sepal width") info <- save_outputs(ggiris, "labels-ylab") - labs <- unlist(lapply(info$layout$annotations, "[[", "text")) - expect_identical(sort(labs), c("Petal.Width", "sepal width")) + labs <- c(info$layout$xaxis$title, info$layout$yaxis$title) + expect_identical(labs, c("Petal.Width", "sepal width")) }) # TODO: why is this failing on R-devel??? diff --git a/tests/testthat/test-ggplot-legend.R b/tests/testthat/test-ggplot-legend.R index 958799dc1f..f161577c74 100644 --- a/tests/testthat/test-ggplot-legend.R +++ b/tests/testthat/test-ggplot-legend.R @@ -30,9 +30,9 @@ test_that("Discrete colour and shape get merged into one legend", { nms, paste0("(", d$vs, ",", d$cyl, ")") ) a <- info$layout$annotations - expect_match(a[[3]]$text, "^factor\\(vs\\)") - expect_match(a[[3]]$text, "factor\\(cyl\\)$") - expect_true(a[[3]]$y > info$layout$legend$y) + expect_match(a[[1]]$text, "^factor\\(vs\\)") + expect_match(a[[1]]$text, "factor\\(cyl\\)$") + expect_true(a[[1]]$y > info$layout$legend$y) }) From c2fa0a17f9f661861feeb0ffc9ceda5dd8fc46cf Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Thu, 5 May 2016 12:42:00 +1000 Subject: [PATCH 3/6] fix typo --- tests/testthat/test-cookbook-axes.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-cookbook-axes.R b/tests/testthat/test-cookbook-axes.R index f7f546c189..e01117c173 100644 --- a/tests/testthat/test-cookbook-axes.R +++ b/tests/testthat/test-cookbook-axes.R @@ -107,7 +107,7 @@ no.x.title <- bp + test_that("coord_fixed(ratio)", { info <- expect_traces(no.x.title, 1, "no-x-title") - expect_identical(info$xaxis$title, "") + expect_identical(info$layout$xaxis$title, "") }) # Also possible to set the axis label with the scale From 64f9170de5436050c885dfb722acd6d093dad510 Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Thu, 5 May 2016 15:14:50 +1000 Subject: [PATCH 4/6] translate titlefont --- R/ggplotly.R | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/R/ggplotly.R b/R/ggplotly.R index 1b970e0e68..7c4b7ce3b3 100644 --- a/R/ggplotly.R +++ b/R/ggplotly.R @@ -353,7 +353,7 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A ticklen = unitConvert(theme$axis.ticks.length, "pixels", type), tickwidth = unitConvert(axisTicks, "pixels", type), showticklabels = !is_blank(axisText), - tickfont = text2font(axisText, "height"), + tickfont = text2font(axisText, type), tickangle = - (axisText$angle %||% 0), showline = !is_blank(axisLine), linecolor = toRGB(axisLine$colour), @@ -364,9 +364,8 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A gridwidth = unitConvert(panelGrid, "pixels", type), zeroline = FALSE, anchor = anchor, - # if not facets then use Plotly axis titling mechanism - # see https://github.com/ropensci/plotly/issues/510 - title = axisTitleText + title = axisTitleText, + titlefont = text2font(axisTitle) ) # convert dates to milliseconds (86400000 = 24 * 60 * 60 * 1000) # this way both dates/datetimes are on same scale @@ -413,8 +412,9 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A if (xy == "y" && inherits(p$facet, "grid")) { gglayout$margin$r <- gglayout$margin$r + stripSize } - # draw axis titles as annotations - # (plotly.js axis titles aren't smart enough to dodge ticks & text) + # facets have multiple axis objects, but only one title for the plot, + # so we empty the titles and try to draw the title as an annotation + gglayout[[axisName]]$title <- "" if (nchar(axisTitleText) > 0) { # npc is on a 0-1 scale of the _entire_ device, # but these units _should_ be wrt to the plotting region @@ -432,13 +432,6 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A } } } - - if (has_facet(p)) { - # turn off plotly axis titles - # since we need special treatment for facets - gglayout[[axisName]]$title <- "" - } - } # end of axis loop xdom <- gglayout[[lay[, "xaxis"]]]$domain From d0c3057e0298625bf8c4fd8c0d672dfa939b49ba Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Thu, 5 May 2016 15:43:28 +1000 Subject: [PATCH 5/6] remove every axis title; account for interior strips in facet_wrap() --- R/ggplotly.R | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/R/ggplotly.R b/R/ggplotly.R index 7c4b7ce3b3..2e9556b9d5 100644 --- a/R/ggplotly.R +++ b/R/ggplotly.R @@ -277,6 +277,7 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A theme[["strip.text.x"]] %||% theme[["strip.text"]], "npc", "height" ) + panelMarginY <- panelMarginY + stripSize # space for ticks/text in free scales if (p$facet$free$x) { axisTicksX <- unitConvert( @@ -307,7 +308,6 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A rep(panelMarginX, 2), rep(panelMarginY, 2) ) - doms <- get_domains(nPanels, nRows, margins) for (i in seq_len(nPanels)) { @@ -414,7 +414,6 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A } # facets have multiple axis objects, but only one title for the plot, # so we empty the titles and try to draw the title as an annotation - gglayout[[axisName]]$title <- "" if (nchar(axisTitleText) > 0) { # npc is on a 0-1 scale of the _entire_ device, # but these units _should_ be wrt to the plotting region @@ -432,6 +431,11 @@ gg2list <- function(p, width = NULL, height = NULL, tooltip = "all", source = "A } } } + + if (has_facet(p)) { + gglayout[[axisName]]$title <- "" + } + } # end of axis loop xdom <- gglayout[[lay[, "xaxis"]]]$domain From b128922ec2588965cf9f20efd43ee5ad80ac7aff Mon Sep 17 00:00:00 2001 From: Carson Sievert Date: Thu, 5 May 2016 16:09:27 +1000 Subject: [PATCH 6/6] bump version; update news --- DESCRIPTION | 2 +- NEWS | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/DESCRIPTION b/DESCRIPTION index 6bf5f1986f..c31479e423 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: plotly Title: Create Interactive Web Graphics via 'plotly.js' -Version: 3.5.4 +Version: 3.5.5 Authors@R: c(person("Carson", "Sievert", role = c("aut", "cre"), email = "cpsievert1@gmail.com"), person("Chris", "Parmer", role = c("aut", "cph"), diff --git a/NEWS b/NEWS index c2372258b3..a21e866aac 100644 --- a/NEWS +++ b/NEWS @@ -1,3 +1,15 @@ +3.5.5 -- 5 May 2016 + +CHANGES: + +ggplotly() will now use plotly's layout.axisid.title (instead of +layout.annotations) for axis titles on non-faceted plots. +This will make for a better title placement experience (see #510). + +BUG FIX: + +Space for interior facet_wrap() strips are now accounted for. + 3.5.4 -- 5 May 2016 BUG FIX: