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

[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 this issue Nov 2, 2024 · 1 comment
Assignees

Comments

@trekonom
Copy link
Contributor

trekonom commented Nov 2, 2024

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

  • using unit = "in" as default
  • checking or making sure that the unit is a vector of length 1

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.

@trekonom trekonom changed the title Add a unit= argument to body_add_gg and body_add_img (and all other functions which use inches as default (: ) [Feature Request] Add a unit= argument to body_add_gg and body_add_img (and all other functions which use inches as default (: ) Nov 2, 2024
@davidgohel
Copy link
Owner

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.

trekonom added a commit to trekonom/officer that referenced this issue Nov 9, 2024
…ell as body_add.gg and body_add.plot_instr.

* Add tests to check that plot size matches the chosen units.

Closes davidgohel#626.
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

No branches or pull requests

2 participants