Skip to content
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

strange behavior with the margin of legend.text for R3.5 #2943

Closed
earthcli opened this issue Oct 16, 2018 · 12 comments
Closed

strange behavior with the margin of legend.text for R3.5 #2943

earthcli opened this issue Oct 16, 2018 · 12 comments
Milestone

Comments

@earthcli
Copy link

@earthcli earthcli commented Oct 16, 2018

I have updated my R to version 3.5. There is too small margin for legend.text when using theme(legend.title=element_blank()), while it is normal without adding this line code.

require(ggplot2)
df <- reshape2::melt(outer(1:4, 1:4), varnames = c("X1", "X2"))
p1 <- ggplot(df, aes(X1, X2)) + geom_tile(aes(fill = value))
p1+scale_fill_continuous(guide = guide_legend())
# there is normal spacing (margin) for legend.text

p1+scale_fill_continuous(guide = guide_legend())+
  theme(legend.title=element_blank())
# there is too small spacing (margin) for legend.text
@clauswilke
Copy link
Member

@clauswilke clauswilke commented Oct 16, 2018

Here is a reprex. The problem is that the default legend.spacing.x is calculated based on the title font size, and when there is no title the font size is zero. Workaround is to set legend.spacing.x manually. A fix could be to look for the default font size before setting it to zero, or just setting it to 11pt instead of 0.

library(ggplot2)
df <- reshape2::melt(outer(1:4, 1:4), varnames = c("X1", "X2"))
p1 <- ggplot(df, aes(X1, X2)) + geom_tile(aes(fill = value))
p1+scale_fill_continuous(guide = guide_legend())

p1+scale_fill_continuous(guide = guide_legend())+
  theme(legend.title=element_blank())

p1+scale_fill_continuous(guide = guide_legend())+
  theme(legend.title=element_blank(),
        legend.spacing.x = unit(5.5, "pt"))

Created on 2018-10-15 by the reprex package (v0.2.1)

@earthcli
Copy link
Author

@earthcli earthcli commented Oct 16, 2018

Thanks, this is exactly what I am looking for.
I have worked around by using theme(legend.title=element_text(color="transparent"))

@thomasp85 thomasp85 added this to the ggplot2 3.2.0 milestone Apr 11, 2019
@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 23, 2019

My feeling is that it would be better to make it dependent on the legend text size rather than the legend title... that would probably slightly change a lot of visualisations... @clauswilke what is your feeling about the best fix? I personally don't think we should be too afraid of slight visual changes for the greater good

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Apr 23, 2019

I wouldn't object to minor visual changes, however I think your proposed solution doesn't get to the heart of the problem, and similar issues could continue to pop up (e.g., if somebody sets the legend text to element_blank()).

The problem is that element_blank() doesn't define a font size and the legend drawing code doesn't realize that. A proper fix would address that issue. I'll take a look.

Note a broadly related issue we have forgotten but maybe should tackle at the same time: #2728

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 23, 2019

I think setting legend text to element_blank would be a non-issue as the spacing would then be invisible (the reason for proposing that approach)

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Apr 23, 2019

No, it's a little more complicated here, because we need to guess the title font size to calculate the default legend spacing:

ggplot2/R/guide-legend.r

Lines 330 to 336 in f79712f

title_fontsize <- title.theme$size %||% calc_element("legend.title", theme)$size %||% 0
# gap between keys etc
# the default horizontal and vertical gap need to be the same to avoid strange
# effects for certain guide layouts
hgap <- width_cm(theme$legend.spacing.x %||% (0.5 * unit(title_fontsize, "pt")))
vgap <- height_cm(theme$legend.spacing.y %||% (0.5 * unit(title_fontsize, "pt")))

One simple option would be to replace the 0 at the end of line 330 with some positive value, e.g. the legend title font size in theme_grey().

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 23, 2019

I may be getting tired, but I still don’t see the benefit of using the title size as opposed to the label size

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Apr 23, 2019

I may be getting tired, but I still don’t see the benefit of using the title size as opposed to the label size

My point was that we're dealing with two separate issues. One is which theme element to draw the font size from. It's currently the legend title, but it could also be the legend text. I have no strong opinion about that. The other is what happens if that element is blank. We still need a default then either way. Currently the default is 0, and that was probably a bad choice when I made it.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 23, 2019

Ok, then my original point was that the default value is ok to be zero if we rely on the legend text, because any spacing will be invisible no matter what (as the text is not there to mark the other side of the spacing)

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Apr 23, 2019

Unfortunately the spacing variables are used in a couple of different contexts (this really needs to be disentangled at some point) and I don't think we can rely on this assumption. See also this comment:

ggplot2/R/guide-legend.r

Lines 333 to 334 in f79712f

# the default horizontal and vertical gap need to be the same to avoid strange
# effects for certain guide layouts

I think using a non-zero hardcoded default value is likely going to be the safest option, regardless of where we take the fontsize from otherwise.

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 23, 2019

ok, I trust you on this (and most other things tbf :-) )

@lock
Copy link

@lock lock bot commented Oct 26, 2019

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 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants