Skip to content

Allowed for user defined axis positions. Closes #2029 #2039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

moutikabdessabour
Copy link
Contributor

@moutikabdessabour moutikabdessabour commented Oct 8, 2021

Now the user can define the axis positions directly from ggplot.

library(plotly)
library(ggplot2)

p <- ggplot(mtcars) +
  geom_point(aes(disp, mpg)) +
  scale_x_continuous(position = 'top')

ggplotly(p)

Screen Shot 2021-10-08 at 9 05 15 AM

the side child element of axisObj was not being set making plotly use the defaults. I added the entry and used ggplot's get_scales function to get the scale for each axis then the position. @cpsievert can you please approve this PR? the tests that failed are related to a missing api key

@moutikabdessabour moutikabdessabour linked an issue Oct 8, 2021 that may be closed by this pull request
Copy link
Collaborator

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moutikabdessabour correctly supporting this feature is a lot more complicated than this due to spacing interactions with titles, labels, etc. I don't think adding this feature (without doing it properly, which will be very difficult) is a good idea since folks can already achieve the same by doing this:

p <- ggplot(mtcars) + geom_point(aes(disp, mpg))
ggplotly(p) %>% 
  layout(xaxis = list(side = "top"))

If you're curious, I worked on #929 a while ago which added this as well as subtitles/captions/footnotes/etc, but I believe funding for my work ended in the middle of the work, so I didn't finish it.

I recommend reaching out to me before working on anymore issues since I can help assess how difficult it will be to do correctly, whether it seems worth doing, and whether there is prior art out there than you can learn from (@jackparmer).

@moutikabdessabour
Copy link
Contributor Author

moutikabdessabour commented Oct 8, 2021

@cpsievert So should I continue working on it? when first working on this issue I changed some values that only modified the plot margins the ones that have if(is_x) .....

@cpsievert
Copy link
Collaborator

cpsievert commented Oct 8, 2021

I would recommend not working on it for now. Here's some other issues I've labelled good for 1st time contributor good-for-first-time-contribution . We can certainly add to that list of you have other issues in mind, just check in with me before starting the work

@cpsievert
Copy link
Collaborator

Closing since #929 is closer to a proper implementation of this feature

@cpsievert cpsievert closed this Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing position of axis labes does not work
2 participants