Settings legend#536
Conversation
|
Sorry for being slow here; |
There was a problem hiding this comment.
This looks great, @vincentarelbundock. Sorry it's taken me soo long to get to it. Only minor comments/nits and then I think we can merge.
(Aside: Man, some of this legend code is unwieldy. It sort of grew up organically as I was finding one issue and plugging another. I'd love to take another crack at simplifying the code at some point...)
R/legend.R
Outdated
| # | ||
|
|
||
| # Unit conversion helpers (used extensively throughout legend positioning) | ||
| lines_to_npc = function(val) { |
There was a problem hiding this comment.
For consitency, I think this function should have the "_x" suffix.
| env2env( | ||
| settings, | ||
| environment(), | ||
| c( |
There was a problem hiding this comment.
Nit: can we make these alphabetic for easy tracking and editing?
R/legend_gradient.R
Outdated
| @@ -0,0 +1,229 @@ | |||
| # Draw vertical gradient legend labels, ticks, and title | |||
There was a problem hiding this comment.
Nit: Can these helper functions come after the main function furthr below (with the params)?
| @@ -68,29 +154,28 @@ draw_multi_legend = function( | |||
| ) | |||
| position = "right!" | |||
| } | |||
|
|
|||
| ## FIXME: current logic only works for "right!"/"left!" legend | |||
There was a problem hiding this comment.
I don't think this FIXME comment should have been deleted. It's something we may want to revist in the future.
|
Thanks for the review! Fixed and merged. |
|
Thanks! Just one minor request for the future: please use Squash and Merge. That way it's much easier for me to keep track of PR-based regressions and contributions. |
2 participants
Continued work on removing core computation steps from the
tinyplot.Rfile. Here, I created helper functions to draw the legends, and kept only minimal machinery insidetinyplot().I also reorganized the code a bit, created a couple helper functions to reduce duplication, and renamed a couple things for consistency and clarity.
I'm sure there are improvements available, but the goal is separation of concern. Then, we can think about simplification/performance/clarity.
Let me know what you think.