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

More aesthetics for geom_label() #5454

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Conversation

steveharoz
Copy link
Contributor

@steveharoz steveharoz commented Oct 4, 2023

Updates to geom_label()'s aesthetics and parameters

  1. The legend keys for geom_label() now have the same rounded rectangle shape and aesthetics as the labels. (See questions below)

Old: image New: image

  1. Deprecated label.size. Use linewidth instead, which is more consistent with the rest of ggplot. If linewidth is not set, it'll fall back to label.size, so nothing should break. Update: setting up a soft deprecation fallback was too messy when going from an argument to an aesthetic, so I fully deprecated label.size.

  2. New aesthetic: linetype for the label border. Default: "solid". Addresses geom_label doesn't allow for varying line types #5365

  3. New parameter: border_colour is the color of the label's border and is decoupled from the text color. If the value is NULL or not set, it will fall back to the text color, which is how it worked previously. Hopefully, that will avoid any breaking changes.

  4. I added three examples to the docs. The mtcars graphs are too crowded to show off all the new aesthetics, so I made a dummy dataset for the latter two:

ggplot(mtcars, aes(wt, mpg, label = rownames(mtcars))) +
geom_label(
  aes(fill = factor(cyl), linetype = qsec < 15),
  border_colour = "black", color = "white", linewidth = 1) +
scale_linetype_manual(values=c("solid", "blank"), limits = TRUE, labels = "1/4 mi < 15s", name = NULL)

image

# Add aesthetics to the border for geom_label
data.frame(x = 1:10, y = 1:10) |>
ggplot() +
  geom_label(aes(
    label=month.abb[x],
    x=x,
    y=y,
    color = factor(x%%3),
    linewidth = x%%2,
    linetype = factor(x%%3)),
    fill = NA) +
scale_linewidth(range=c(0.5, 1.5)) +
scale_linetype_manual(values=c("solid", "blank", "dotted"))

image

# Override the border color
data.frame(x = 1:10, y = 1:10) |>
ggplot() +
  geom_label(aes(
    label=month.abb[x],
    x=x,
    y=y,
    color = factor(x%%3),
    linewidth=x%%2,
    linetype=factor(x%%3)),
    border_color = "red",
    fill=NA) +
scale_linewidth(range=c(0.5, 1.5)) +
scale_linetype_manual(values=c("solid", "blank", "dotted"))

image

Questions:

  1. Does the deprecated messaging look right for label.size?
  2. There is a bug where the key_glyph is cropped at the bottom. Can anyone help with that?
  3. Are the aliases border_colour/border_color dealt with appropriately?

Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, Steve!

I like the intent of the PR and the comments I've made are mostly about polishing and conventions. I don't think ggplot2 has settled on how paired aesthetics (colour/border_colour) should be implemented yet, but I can bring it up with Thomas soon.

R/geom-label.R Outdated Show resolved Hide resolved
R/geom-label.R Outdated Show resolved Hide resolved
R/geom-label.R Outdated
Comment on lines 74 to 75
border_colour = NULL,
border_color = border_colour) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to add these to the formals of geom_label() as well? It might also be best to standardise to UK spelling immediately in geom_label(). In addition, I think ggplot2 has settled at using dot.case instead of snake_case for most arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 3002125

As for dot.case vs. snake_case, it's not consistent, so I'm not sure. nudge_x and nudge_y are arguments that use snake_case in that same function. And the function name itself is geom_label. I'll lean towards underscore unless it's clearly settled. If so, happy to change it :)

Copy link
Member

Choose a reason for hiding this comment

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

argument names should use dot.case...

Copy link
Member

Choose a reason for hiding this comment

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

Also, if you can think up a name that doesn't have variation between UK/US spelling it would be preferable because we really have gripes with the decision to support both. While we can't backtrack on that decision we can attempt to limit new entrants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I updated to dot.case.

  2. Yeah, I see how the redundant variables are a headache. I couldn't come up with a good alternative to term for colour/color, so I'm copying what the rest of ggplot does. Open to alternatives!

R/geom-label.R Outdated Show resolved Hide resolved
R/geom-label.R Outdated Show resolved Hide resolved
R/legend-draw.R Outdated Show resolved Hide resolved
steveharoz and others added 6 commits October 5, 2023 11:15
Co-authored-by: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
Co-authored-by: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
@steveharoz steveharoz requested a review from teunbrand October 5, 2023 17:46
@steveharoz
Copy link
Contributor Author

I've updated the comment at the beginning. See the two remaining questions.

@olivroy
Copy link
Contributor

olivroy commented Oct 6, 2023

I was looking for that exactly! Thank you very much.

I guess that before this PR, it was not possible to only apply the color to the label box?

After this PR, some work will be required in ggrepel to make geom_label_repel() understand that?

@teunbrand
Copy link
Collaborator

teunbrand commented Oct 6, 2023

After this PR, some work will be required in ggrepel to make geom_label_repel() understand that?

The ggrepel package has always been free to do whatever it wants with labels, so there'd be no need to wait on this PR for that.

steveharoz requested a review from teunbrand 20 hours ago

I have to probe Thomas' wisdom on some points, I'll get back on this on tuesday/wednesday.

As a quick comment though: I don't think you'd need all the examples to show all the aesthetics. Most people should be familiar with what 'linewidth' is doing and how this might apply to textboxes. The border_colour and how it interacts with colour probably is worth having an example about.

@steveharoz
Copy link
Contributor Author

steveharoz commented Oct 6, 2023

@teunbrand Thanks. Fair point about the examples. How about these three? The last could be simpler, but I think it's a use case some people may want to copy.

p = ggplot(mtcars, aes(wt, mpg, label = rownames(mtcars)))

# If border.colour is NULL or not set, the border will use the text color
p + geom_label(aes(color = factor(cyl)))

image

# Alternatively, border.colour can have a static value
p + geom_label(aes(color = factor(cyl)), border.colour = "black")

image

# Use linetype and linewidth aesthetics to add highlights
p + geom_label(
  aes(fill = factor(cyl), linetype = qsec < 15),
  border.colour = "black", color = "white", linewidth = 1) +
scale_linetype_manual(values=c("solid", "blank"), limits = TRUE, labels = "1/4 mi < 15s", name = NULL)

image

@steveharoz steveharoz requested a review from thomasp85 October 9, 2023 12:23
Copy link
Collaborator

@teunbrand teunbrand left a comment

Choose a reason for hiding this comment

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

Hi Steve,

I had a chat with Thomas and we agree that PR's direction is great.
I've left some minor formatting comments.
About the legend clipping, I think legends should be slightly smarter about determining the size of the keys, so I'll try to fix that in a seperate PR first before we can merge this one.

R/geom-label.R Outdated Show resolved Hide resolved
R/geom-label.R Outdated Show resolved Hide resolved
R/geom-text.R Outdated Show resolved Hide resolved
R/geom-text.R Outdated Show resolved Hide resolved
steveharoz and others added 4 commits October 10, 2023 12:16
Co-authored-by: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
Co-authored-by: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
Co-authored-by: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
Co-authored-by: Teun van den Brand <49372158+teunbrand@users.noreply.github.com>
@steveharoz
Copy link
Contributor Author

Thanks @teunbrand!

@teunbrand
Copy link
Collaborator

If #5465 gets merged, we can fix the key glyph cropping issue.

@steveharoz steveharoz marked this pull request as draft October 10, 2023 15:55
@steveharoz
Copy link
Contributor Author

I'm moving this PR back to a draft to consider to make border.colour into a propper aesthetic (https://blueviewer.pages.dev/view?actor=mjskay.com&rkey=3kbfolzxlpq2s). That would also allow for not needing to worry about the weird special fallback case where NULL uses the text color. Instead, both border.colour and (text)color could be mapped the same way.

@teunbrand
Copy link
Collaborator

teunbrand commented Oct 10, 2023

Yeah that is exactly what Thomas and I were discussing, but we both thought making border.colour a proper aesthetic opens the floodgates for a new level of complexity that is difficult to understand and teach people. Here, as @bwiernik points out in the thread you linked, one could override a scale's aesthetics argument and that will work out fine. However, in #5423 there would be whisker_linetype aesthetics and linetype scales don't have an aesthetics argument. In fact, I'm planning to roll back the proper aesthetics in #5423 to fixed parameters because of this.

That would also allow for not needing to worry about the weird special fallback case where NULL uses the text color.

The mechanism already has precedence in geom_boxplot() with outlier.colour etc.

@bwiernik
Copy link
Contributor

Personally, I find fixed parameters extremely frustrating because very quickly collaborators or audiences for plots that use them start to ask for data-dependent customization of them (eg, making the linetype for the whiskers match the linetype for the box or vary depending on whether they cross a threshold).

It would seem that a better solution in the case of #5423 would be to add the aesthetics argument to linetype scales

@smouksassi

This comment was marked as off-topic.

@teunbrand

This comment was marked as off-topic.

@teunbrand

This comment was marked as outdated.

@olivroy
Copy link
Contributor

olivroy commented Dec 15, 2023

Is there any chance this will be merged for this release? or will this get looked at after release?

@teunbrand
Copy link
Collaborator

@olivroy This PR is currently set as a draft (so we cannot merge it) and the release process has been kicked off.
While ideally this PR would have been flagged in the 3.5.0 milestone, it is currently not too late, as we haven't started the reverse dependency check yet (see #5588). However, we will start that soon, so I would expect it will not be included in 3.5.0.

@steveharoz
Copy link
Contributor Author

Been busy with other things. But I'll try to work on it this weekend.

@teunbrand
Copy link
Collaborator

There is no rush, I'm reasonably sure that the upcoming release would have to be patched afterwards so we can include it in that release. If someone needs these features right this instant, they can extend geom_label() if they want.

@teunbrand
Copy link
Collaborator

teunbrand commented Nov 20, 2024

Hi @steveharoz, are you still interested in finishing this PR? If you're busy with other things that is fine too, and we'd be happy to take over from here.

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.

6 participants