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

add logo to bottom right of plots #2

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

add logo to bottom right of plots #2

wants to merge 5 commits into from

Conversation

kweav
Copy link
Contributor

@kweav kweav commented Jan 27, 2025

On top of having some AnVIL specific color palettes available, adding the possibility to include an AnVIL logo (full or just the anvil image) on the bottom right of plots. Using Howard's example from the hutchplot package here as well.

@kweav kweav requested a review from avahoffman January 29, 2025 21:35
@kweav kweav changed the base branch from colorPalettes to main February 4, 2025 21:56
@kweav
Copy link
Contributor Author

kweav commented Feb 4, 2025

@avahoffman latest commit adds a rendered README (with the adding an AnVIL logo in the example commented out since that's not merged just yet)

@avahoffman
Copy link

Great! Can you make sure this is up to date with main?

@kweav
Copy link
Contributor Author

kweav commented Feb 4, 2025

Up to date with main now! Thanks for the reminder!

@avahoffman
Copy link

You might need to push to origin (this PR) still :)

@kweav
Copy link
Contributor Author

kweav commented Feb 5, 2025

Yeah -- sorry about that -- pushed to origin as well now

Copy link

@avahoffman avahoffman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to have pictures of the rendered plots!

#' Higher values shift towards the right, lower values shift towards the left
#' @param y_unit Numeric value indicating the amount of vertical shifting.
#' Greater values result in downward shifting, smaller values result in an upward shifting
#' @param height_unit Numeric value specifying height

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' @param height_unit Numeric value specifying height
#' @param height_unit Numeric value specifying height (size) of the logo

# Render raster object
render_png <- function(file_name,
x_unit = NULL, y_unit = NULL,
height_unit = NULL, width_unit = NULL){

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to have the width_unit arg? Looks like you aren't using it. Might swap height_unit for an arg like size

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ported over the function from Howard's hutchplot: https://github.com/FredHutch/hutchplot/blob/main/R/logo.R
I kept it even though I recognize the width unit isn't used clearly because of a review comment asking for those arguments (FredHutch/hutchplot#3 (comment)). Happy to change/simplify/whatever makes sense

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the previous comments that the user should be able to adjust size. However, we should clean up args that aren't being used. I'd either:

  • Remove width_unit as an argument
  • Make it so that width_unit is used inside render_png and anvil_logo

Up to you, whichever you prefer! Personally I think a single argument like size might be more intuitive and also prevent folks from "warping" the logo dimensions.

plot(anvil_palette_discrete)
```

<img src="man/figures/README-unnamed-chunk-4-1.png" width="100%" />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you intend for there to be figures here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully they are there now! Sorry I missed adding those

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

Successfully merging this pull request may close these issues.

2 participants