search

mbojan / alluvial

Alluvial diagrams
Other
139 stars 26 forks source link

titles for alluvial() #35

Closed corybrunson closed 7 years ago

corybrunson commented 7 years ago

The two functions plot their elements differently, and the title parameter is only available in alluvial_ts(). Is there a specific reason that titles haven't been implemented for alluvial()? I want to respond to this question on SO, and i was going to suggest using mtext() in tandem with the ylim_offset parameter. But it would be straightforward to add a title parameter (or maybe main, but that would be inconsistent with alluvial_ts()) and pass it to the main parameter in the plot() call, so i'm checking to see if that would be desirable. Sound good?

mbojan commented 7 years ago

Yeah... I did not implement title and so on exactly because this can be added with mtext(). I'd suggest extactly that. Making a space for a title would require adjusting mar argument (passed to par()). Tweaking the offsets should not be necessary.

A title or main argument could be added anyway though...

mbojan commented 7 years ago

I already started replying if it is OK with you.

corybrunson commented 7 years ago

Of course! Thanks for doing so.

Yeah, there's a slippery slope risk here, of adding more and more parameters from plot(), which is sometimes done using .... It makes sense to limit the plotting passes to the necessary ones; and, to me, that suggests that it's better to synchronize the functions by using title.

If a title parameter is included, do you think the mar default should automatically change? (I'm playing with a "title" branch on my fork.)

mbojan commented 7 years ago

Yeah, there's a slippery slope risk here, of adding more and more parameters from plot(), which is sometimes done using .... It makes sense to limit the plotting passes to the necessary ones; and, to me, that suggests that it's better to synchronize the functions by using title.

Yep, makes sense.

If a title parameter is included, do you think the mar default should automatically change? (I'm playing with a "title" branch on my fork.)

The default mar (2,1,1,1) should accomodate a single-line title when mtext is used with line=3.

corybrunson commented 7 years ago

You're right! I thought i'd generated an example that showed inadequate space for the title, but it seems to work on the home examples. I'll send a PR momentarily.

corybrunson commented 7 years ago

Sorry, new etiquette question: I'm using a more recent version of Roxygen (6.0.1 versus 5.0.1), which results in other .Rd files being changed as well as "DESCRIPTION". Is that OK for a PR?

mbojan commented 7 years ago

You're right! I thought i'd generated an example that showed inadequate space for the title, but it seems to work on the home examples. I'll send a PR momentarily.

Great, thanks.

Sorry, new etiquette question: I'm using a more recent version of Roxygen (6.0.1 versus 5.0.1), which results in other .Rd files being changed as well as "DESCRIPTION". Is that OK for a PR?

Yes, go ahead.

mbojan commented 7 years ago

Done in #36