Add support for alternative axis sides in ggplotly #813

alanocallaghan · 2016-11-23T22:40:12Z

This is a fairly straightforward edit and shouldn't break any existing functionality. It just adds support for the new axis locations (top and right) in ggplot2 2.2.0 which already have support in plotly.js

talgalili · 2016-11-24T06:03:02Z

Great - thanks for your work @alanocallaghan

alanocallaghan · 2016-11-24T23:34:37Z

Cheers Tal

Addresses #808

cpsievert · 2016-11-25T17:24:41Z

Thanks @alanocallaghan, this is a nice start 🍻 , but it will have to be generalized if we want it to work with facets.

To do so, I think you can essentially copy the same logic over to the axis object generation step

And before I merge anything, it'd be great to have some tests!

alanocallaghan · 2016-11-26T11:39:52Z

Thanks for getting back quickly. Moving the logic seems to work fine.

Regarding facets, some of the other new additions in ggplot 2.2.0 might also require small edits, but I've not tested any yet. Probably one to watch out for, though

Regarding tests, I've written simple tests that verify whether the position moves and ensure 1 trace

Cheers

alanocallaghan · 2016-11-26T15:25:16Z

I should say that if you would like further tests or tests with more complex plots, just say the word

cpsievert · 2016-11-27T22:41:06Z

Nice work! More tests are always good...lets put a test with facets in there as well

alanocallaghan · 2016-11-28T23:44:18Z

Unfortunately, the facet functionality technically works (http://i.imgur.com/s2PIqXA.png) but requires a bit more work...

talgalili · 2017-02-08T16:59:30Z

Hi @cpsievert - any chance you'll give this a look?
Adding alternative location for the axis is a feature various people have asked for.

Thanks.

alanocallaghan · 2017-02-08T18:13:23Z

I can pick this up again tonight and resolve the failing test and conflicts; it will still look awful with facets for the time being

alanocallaghan · 2017-02-08T22:04:17Z

@cpsievert or any others

The test errors are now not related to my PR (they seem to be in the master branch). I can pull in the master when the current issue is resolved.

As for facet functionality, for the x axis on the top, I'm not sure of the logic needed to pad the facet "strip" (made by make_strip_rect) in order to show the axis in this position.

For the y axis, the position is a bit trickier. You need to move the axis from the leftmost plot to the rightmost plot... Not sure where the logic for this is found.

For both plots, you also need to move the axis label.

Any pointers on where to look would be great, the ggplotly code is quite dense as it's not split into many functions.

alanocallaghan · 2017-02-18T11:26:03Z

The parameter is axis anchoring, eg

layout(yaxis = list(anchor = "x2"))

alanocallaghan · 2017-02-21T22:51:24Z

Made some progress, but unfortunately the "annotation" x/y axis labels and striptext are difficult to move. I think the best way forward is to extend the axis ticks, via axisObj$ticklen, beyond the striptext. This also handles moving the tick text.

alanocallaghan · 2017-02-21T23:04:21Z

I've managed to get this to work reasonable well for a few different facets, shown in these images (http://imgur.com/a/DYCDt). Will write some tests for these

alanocallaghan · 2017-02-21T23:28:11Z

y axis labels are not the right way up...

alanocallaghan · 2017-02-22T00:19:45Z

Should be good to go once I write tests for all of the changes made

talgalili · 2017-02-22T21:25:46Z

Thank you for all your great work @alanocallaghan !

alanocallaghan · 2017-02-22T22:15:22Z

Tests took a while to get right (can't run them locally), should be golden now

alanocallaghan · 2017-02-27T16:15:49Z

@cpsievert can you have a look whenever you have time? Thanks

ck37 · 2017-03-02T20:45:45Z

Really interested in this patch. When I use it to move the x-axis to the top (to avoid the horizontal legend overlapping bug) the x-axis overlaps with the title. Is there any way to fix that?

Code:

    p = plot_ly(ellipse, x = ~x, y = ~y, split = ~source) %>%
      layout(title = "Archeological Geochemistry",
             xaxis = list(title = names(elements)[elements == input$element1],
                          side = "top"),
             yaxis = list(title = names(elements)[elements == input$element2]),
             legend = list(orientation = "h"))

Screenshot: http://imgur.com/a/unwwf

alanocallaghan · 2017-03-02T22:04:34Z

Good spot, here's a reprex equivalent with mtcars:

p <- plot_ly(mtcars, x = ~cyl, y=~mpg, color =~gear) %>%
    layout(title = "Archeological Geochemistry",
         xaxis = list(title = "X Axis Title",
                      side = "top"),
         yaxis = list(title = "Y Axis Title"),
         legend = list(orientation = "h"),
         margin=list(t=100))

alanocallaghan · 2017-03-02T22:10:27Z

This is actually related to plotly itself, while this PR only touches ggplotly. @ck37 you may be best served looking at annotations https://plot.ly/r/text-and-annotations/

cpsievert · 2017-03-09T16:43:13Z

R/ggplotly.R

+
+      ## Move axis and change anchor if necessary
+      if (has_facet(plot)) {
+        if (non_default_side) {


where possible, I prefer:

if (x && y) { }

to

if (x) { if (y) { } }

Thanks, I think I'd just thought the logic was going to be more complex

cpsievert · 2017-03-09T16:45:14Z

R/ggplotly.R

+              (unitConvert(stripText, "pixels", type) * 2.5)
+            if (nRows > 1) {
+              axisObj[["anchor"]] <- "y"
+            } 


What if there are multiple rows?

I think the if statement is actually redundant, since it will always need to be anchored to the first row, whether there is more than one row or not

cpsievert · 2017-03-09T16:50:24Z

tests/testthat/test-ggplot-axis.R

+
+test_that("Axis position moves to right", {
+  p <- p + scale_y_continuous(position="right")
+


The axis currently isn't shown because this PR hasn't yet addressed the issue of adding space for axis ticks/text in the right margin (and removing from the left) as is done here

Fixed, thanks for the tip

alanocallaghan · 2017-03-09T21:03:13Z

Thanks, I'll fix these and do more extensive checks tomorrow

alanocallaghan · 2017-03-09T22:18:56Z

R/ggplotly.R

@@ -599,7 +599,8 @@ gg2list <- function(p, width = NULL, height = NULL,
        if (xy == "x") {
          ## Facet labels are always on top, so add tick length to move past them
          axisObj[["ticklen"]] <- axisObj[["ticklen"]] + 
-            (unitConvert(stripText, "pixels", type) * 2.5)
+            (unitConvert(stripText, "pixels", type) * 3)


This is a magic number. I don't think ticklen can vary dynamically so it has to be large enough to accomodate large and small window sizes. It'll be too long for small window sizes but if it's extreme the user can adjust it using p %>% layout(xaxis=list(ticklen=10), xaxis2=list(ticklen=10), ...).

I'll log an issue on the plotly.js github to see if it would be possible to have ticklen optionally be a proportion of the plot rather than strictly pixels, but otherwise it's a necessary evil

alanocallaghan · 2017-03-15T15:40:54Z

The failure is due to package semver... Going to try re-running to see if it still fails

talgalili · 2017-04-12T08:38:22Z

Hi @alanocallaghan
Any chance this could be resolved?
(once done, heatmaply would be able to support left dendrograms with plot_method="ggplot", which would be a good thing, I believe)

Thanks.

alanocallaghan · 2017-04-12T09:34:41Z

The fail on R-devel seems to be down to Rserve connection to the plotly servers. Regarding this functionality, I can't do much more while #1458 is still open so it's up to @cpsievert where to go from here, though I imagine he's busy with PlotCon around now

talgalili · 2017-04-12T09:49:53Z

Hi @alanocallaghan Just to be sure - I can't seem to find issue #1458, which issue did you mean?

alanocallaghan · 2017-04-12T09:57:53Z

Sorry: plotly/plotly.js#1458

cpsievert · 2017-04-13T21:53:01Z

Don't worry about the test failures @alanocallaghan. I'll have a closer look here next week

cpsievert · 2017-05-11T00:44:43Z

@alanocallaghan Just FYI, #929 should supersede this, but I may use some of your tests -- #929 is a refactoring of the labs() framework which should fix/add support for a bunch of things. Alternative axes are coupled with that logic, so I'll handle it over there.

alanocallaghan · 2021-04-27T21:22:53Z

Closing as superseded

Add support for alternative axis sides in ggplotly

1ae04af

DRY - remove redundant assignment

bb7e07b

Add axis test, refactor axis movement code

b5bbc2a

alanocallaghan added 3 commits November 27, 2016 17:41

Add bottom/left test

369b760

Add default axis code

94ce05b

Fix test message

4edf8e8

alanocallaghan added 2 commits February 8, 2017 20:27

Fix error

21c5712

Fix merge conflict and test error

4379260

Move anchor when non-standard axis position

cbd3077

Move ticks above facet labels when non-standard

304a1ab

Fix merge conflict

ebb135c

Fix axis text rotation

b82036d

alanocallaghan closed this Feb 22, 2017

alanocallaghan reopened this Feb 22, 2017

add tests

92ba0eb

alanocallaghan added 2 commits February 22, 2017 21:34

Fix broken tests

347cd3e

Fix final broken test

00f5bda

Remove comment and add explanation

d3527db

cpsievert reviewed Mar 9, 2017

View reviewed changes

alanocallaghan added 2 commits March 9, 2017 21:51

Add code to pad properly for axis movement

1a3941e

Tweak ticklen

1f6a55e

alanocallaghan commented Mar 9, 2017

View reviewed changes

alanocallaghan closed this Mar 15, 2017

alanocallaghan reopened this Mar 15, 2017

Merge with master

523a860

plotly deleted a comment from KeeonTabrizi Feb 27, 2019

cpsievert mentioned this pull request Feb 27, 2019

support for sec_axis #1051

Open

alanocallaghan closed this Apr 27, 2021


		test_that("Axis position moves to right", {
		p <- p + scale_y_continuous(position="right")

Add support for alternative axis sides in ggplotly #813

Add support for alternative axis sides in ggplotly #813

Conversation

alanocallaghan commented Nov 23, 2016

talgalili commented Nov 24, 2016

alanocallaghan commented Nov 24, 2016

cpsievert commented Nov 25, 2016

alanocallaghan commented Nov 26, 2016

alanocallaghan commented Nov 26, 2016

cpsievert commented Nov 27, 2016

alanocallaghan commented Nov 28, 2016

talgalili commented Feb 8, 2017

alanocallaghan commented Feb 8, 2017

alanocallaghan commented Feb 8, 2017

alanocallaghan commented Feb 18, 2017 • edited Loading

alanocallaghan commented Feb 21, 2017 • edited Loading

alanocallaghan commented Feb 21, 2017 • edited Loading

alanocallaghan commented Feb 21, 2017

alanocallaghan commented Feb 22, 2017

talgalili commented Feb 22, 2017

alanocallaghan commented Feb 22, 2017

alanocallaghan commented Feb 27, 2017

ck37 commented Mar 2, 2017

alanocallaghan commented Mar 2, 2017

alanocallaghan commented Mar 2, 2017 • edited Loading

cpsievert Mar 9, 2017

Choose a reason for hiding this comment

alanocallaghan Mar 9, 2017

Choose a reason for hiding this comment

cpsievert Mar 9, 2017

Choose a reason for hiding this comment

alanocallaghan Mar 9, 2017

Choose a reason for hiding this comment

cpsievert Mar 9, 2017

Choose a reason for hiding this comment

alanocallaghan Mar 9, 2017

Choose a reason for hiding this comment

alanocallaghan commented Mar 9, 2017

alanocallaghan Mar 9, 2017

Choose a reason for hiding this comment

alanocallaghan commented Mar 15, 2017

talgalili commented Apr 12, 2017

alanocallaghan commented Apr 12, 2017

talgalili commented Apr 12, 2017

alanocallaghan commented Apr 12, 2017

cpsievert commented Apr 13, 2017

cpsievert commented May 11, 2017

alanocallaghan commented Apr 27, 2021

alanocallaghan commented Feb 18, 2017 •

edited

Loading

alanocallaghan commented Feb 21, 2017 •

edited

Loading

alanocallaghan commented Feb 21, 2017 •

edited

Loading

alanocallaghan commented Mar 2, 2017 •

edited

Loading