davidgohel / officer

:cop: officer: office documents from R
https://ardata-fr.github.io/officeverse/
Other
602 stars 107 forks source link

[Feature Request] Add a unit= argument to body_add_gg and body_add_img (and all other functions which use inches as default (: ) #626

Open trekonom opened 5 hours ago

trekonom commented 5 hours ago

Hi David,

I'm aware that this FR is a duplicate of PR #319, which was reverted in #322 due to adding three copies of the plot when the units= argument was not set (as also reported in #323).

I still think a unit(s)= argument would be a handy feature, and after looking at the implementation proposed in #319, I think the problem can be easily fixed.

The problem with #319 is that the default was set to units=c("in", "cm", "mm"). As a result, three copies of the plot were created via external_img when the units= argument was not set. Side note: Nowadays, you already get an error in ggsave or agg_png if you pass a vector with multiple units, which was not the case in older versions of ggplot2, e.g. I checked with ggplot2 3.2.0.

However, I think a working solution can be easily implemented by

Additionally, since the problem was caused by external_img creating multiple copies when a vector of length > 1 is passed to the unit= argument, I'm wondering if a check should be added to external_img as well?

Finally, I'm wondering about the "correct" naming for a unit(s)= argument, i.e. ragg::agg_png (or ggsave) uses units= while the officer package already uses unit=. For consistency, sticking to the officer naming convention is probably the way to go. However, in this case it would be nice to add an informative (error) message if a user uses units= instead.

What do you think of this suggestion? Feel free to assign it to me.

davidgohel commented 4 hours ago

Hello @trekonom

No problem, you can propose a PR, I'll happily review it. You can also ask if any help is needed. :)

[...] the "correct" naming for a unit(s)= argument [...]

yeahhh, to me it should be unitas we express only one but I agree with you.