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

The colorbar guide (guide_colourbar) incorrectly uses a vertical unit in a horizontal setting #2397

Closed
clauswilke opened this Issue Jan 11, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@clauswilke
Member

clauswilke commented Jan 11, 2018

The colorbar guide (guide_colourbar) incorrectly uses a vertical unit in a horizontal setting. The simplest way to see this issue is the following experiment: Increase the height of the colorbar title and observe that the guide ticks are moving to the right at the same time.

library(ggplot2)
p1 <- ggplot(iris, aes(Sepal.Width, Sepal.Length, color = Petal.Width)) + geom_point() +
  scale_color_distiller(name = "Petal Width")
p2 <- ggplot(iris, aes(Sepal.Width, Sepal.Length, color = Petal.Width)) + geom_point() +
  scale_color_distiller(name = "Petal\nWidth")
p3 <- ggplot(iris, aes(Sepal.Width, Sepal.Length, color = Petal.Width)) + geom_point() +
  scale_color_distiller(name = "Petal\n\nWidth")

cowplot::plot_grid(p1, p2, p3, nrow = 1)

screen shot 2018-01-11 at 11 31 08 am

The problem is caused by this line and/or this line. The variable vgap is being used as a width. The correct variable here would be hgap. See e.g. this line, which is correct.

@hadley

This comment has been minimized.

Member

hadley commented Jan 12, 2018

In the future can you please make sure one of the commit messages includes Fixes #2397 so the issue is automatically closed?

@hadley hadley closed this Jan 12, 2018

@clauswilke

This comment has been minimized.

Member

clauswilke commented Jan 12, 2018

Hadley, I think there was a misunderstanding. Pull request #2382 does not fix #2397, it only fixes #2260. Issue #2397 (this issue) will require a separate pull request. I haven't done it yet because I'll run into the problem that the vdiffr examples will have to be fixed, and I haven't been able yet to get vdiffr to give me a clean pass on the current examples in the ggplot2 code tree.

Is there a document somewhere that describes exactly which library versions of freetype etc. should be used to run the vdiffr tests for ggplot2?

@batpigandme batpigandme reopened this Jan 12, 2018

@hadley

This comment has been minimized.

Member

hadley commented Jan 12, 2018

Oops, misread sorry.

Probably the best bet for now, is just to run vdiffr locally on master, check in results, then confirm your change doesn't break any new tests. If you tell me it's ok, I'll take your word for it.

(in the long term, @lionel- is going to work on embedding the libraries into vdiffr to avoid this sort of problem, but unfortunately other projects have higher priority right now)

@clauswilke

This comment has been minimized.

Member

clauswilke commented Jan 12, 2018

Ok. One more question: Fixing #2397 is a five-minute effort but will break the vdiffr tests (because spacing of labels will change). Fixing #2398 is a more involved process, because it touches on many parts of the colorbar code. It will possibly break the vdiffr tests again. Should I do the two separately or at once? I'm leaning towards all at once, but I'm not sure how quickly I can get to the more involved part.

@hadley

This comment has been minimized.

Member

hadley commented Jan 12, 2018

It's basically up to you. I know think I can get tidy evaluation working with out too much effort, so I'll push off the release until that happens.

clauswilke added a commit to clauswilke/ggplot2 that referenced this issue Jan 20, 2018

clauswilke added a commit to clauswilke/ggplot2 that referenced this issue May 2, 2018

@hadley hadley closed this in #2415 May 2, 2018

hadley added a commit that referenced this issue May 2, 2018

Colorbar fixes; closes #2397 and #2398 (#2415)
* Fixes #2397

* Consistenly use cm as units throughout. Fixes #2398

* clean up code, use variable names similar to guide-legend.

* update vdiffr templates

* fix bug in guide-text spacing for guide_legend(), harmonize guide_legend()
and guide_colorbar(), tweak default spacings.

* make default hgap and vgap 0.5 * fontsize.

* update NEWS
@lock

This comment has been minimized.

lock bot commented Oct 29, 2018

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/

@lock lock bot locked and limited conversation to collaborators Oct 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.