Unified legend

[grantmcdermott]

Closes #19 (the remaining legend part).

TL;DR This PR unifies the old "legend.position" and "legend.arguments" arguments into a single, flexible legend argument. Some quick examples.

library(plot2)

# Keyword example
plot2(
  Temp ~ Day | Month,
  data = airquality,
  type = "l",
  legend = "bottom!"
)

# More sophisticated example where, instead of a keyword, we pass on a full
# legend function with addition arguments 
plot2(
  Temp ~ Day | Month,
  data = airquality,
  type = "l",
  legend = legend("bottom!", title = "Month of the year", bty = "o")
)

^{Created on 2023-05-26 with reprex v2.0.2}

[grantmcdermott]

@zeileis Sorry that this has taken me so long to get around to. I've been sprinting at work. The legend tweaking also turned out to be a tougher nut to crack than I first expected. I think it's quite stable now---and have added various tests---but would appreciated you casting an eye over it.

[grantmcdermott]

Sorry to nag you @zeileis (especially given how long it took me to get this PR together), but please me know if you get a chance to review this. (Key diff is here.)

No worries if you're busy and I might just merge as-is then. I mostly want to tie a bow on this legend functionality, so I can put out a new release and then give you a chance to work on the faceting code (if that still interests you!)

[zeileis]

Sorry @grantmcdermott for the delay. The kids had chicken pox last month and now we're heading into exam period so time is a bit limited. I'm still interested, though, in moving this forward. I was able to take a bit of time today to play around with this, very nice, thanks for putting so much work into this!

Just a bit of feedback and suggestions for improvement. Mainly I don't like this (but maybe I don't fully understand it):

• Why do you advertise legend = legend(...) as opposed to legend = list(...)? Wouldn't the latter be more standard and easier to explain?

Details:

• I suggest that legend = FALSE should be mapped to legend = "none". Similarly, legend = TRUE should be mapped to legend = NULL.
• Can I suppress the title of the legend somehow? If I set legend = list(title = NULL) I get "by" as the title which is probably not ideal anyway. I suggest to drop the title in that case or use the default title.
• Can I overrule the legend labels? I would have expected that this works for the airquality example: legend = list(legend = month.abb[5:9]).
• Is there a particular reason for not supporting "top!" and "left!" positions?

[grantmcdermott]

Thanks very much for the feedback @zeileis. I'll try to address your comments and ping you again once the revised PR is ready (hopefully some time this week).

PS. Ugh, sorry to hear about the sick kids. We've had a doozy of a spring season this side too.

[grantmcdermott]

Hi @zeileis, thanks again for the suggestions. I think these should all be addressed now, so please free to squash+merge if you're satisfied.

Once that's done, I'll publish a v0.0.3 release on R-universe, then kick it over to you for the faceting stuff.

PS. Here's a quick example below combining several of your suggestions: passing a list instead of a legend function, additional keyword-! support, switching off the legend title with NULL, and support for user supplied labels.

devtools::load_all("~/Documents/Projects/plot2/")
#> ℹ Loading plot2

plot2(
  Temp ~ Day | Month, airquality,
  type = "l", main = "Here is a new plot",
  legend = list("top!", title = NULL, legend = month.abb[5:9])
)

^{Created on 2023-06-18 with reprex v2.0.2}

PPS. A bit more on this point:

Why do you advertise legend = legend(...) as opposed to legend = list(...)? Wouldn't the latter be more standard and easier to explain?

The short reason is that I did this to be consistent with the other arguments that take dedicated functions, e.g. plot2(..., grid = grid(), palette = palette()). But I ofc agree that it's less obvious in the case of a legend, since there's only really one function that we can call. So a regular list is just as good. I've updated the docs to make this equivalence clearer.

[grantmcdermott]

Just a heads-up that I pushed a few extra changes to match @vincentarelbundock's new additions on the main branch and also fix some minor test errors. But from my perspective this PR is good to go now.

[zeileis]

Great, thanks! I'll try to have a closer look in the next days.

For the facets I should have some time in the first two weeks of July.

[grantmcdermott]

Super, thanks @zeileis

If early July is your window for the faceting stuff then we might try to sneak in a few more changes/fixes before "freezing" the main branch for you to have a crack at that. But that's all stuff that we can get to later. Just let us know when you're ready to go ;-)

[zeileis]

This week and next week I have the final exams in several courses plus meetings etc. Starting from July I'm not teaching anymore :sweat_smile:

[grantmcdermott]

Hi @zeileis. Sorry to be a pain about this, but do you think you'll be able to take a look this week? I'm hoping to get the new minor release out for r-universe soon, so I can share with colleagues for a joint project. Again, I'm pretty confident that I addressed all of your earlier concerns; see my previous comment for an example.

If you're tied up with other commitments, then no worries. Though I may just merge as-is to get this updated version on r-universe.

[zeileis]

Additional comment (outside the review): Apologies for the delay. My plans for July collapsed completely. I hope to have some time in Aug/Sep for the facets but I'll let you know when I really know when. Sorry!

[grantmcdermott]

Achim, you're a mensch. (And don't forget that I still owe you for several over-late econometrics TaskView issues.)

I'll have a crack at your other suggestions as soon as I get a chance!

[zeileis]

Thanks for your understanding! And, of course, you don't owe me anything... :-)