-
Notifications
You must be signed in to change notification settings - Fork 332
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
fix: Do not change the default dimensions of altair charts unless they are set with the altair specific options #1646
fix: Do not change the default dimensions of altair charts unless they are set with the altair specific options #1646
Conversation
… set explicitly This will avoid using the knitr default dimensions from other figure formats for altair charts since altair already has sensible and easily overidable defaults
@@ -767,22 +767,18 @@ eng_python_autoprint <- function(captured, options) { | |||
|
|||
} else if (eng_python_is_altair_chart(value)) { | |||
|
|||
# set width if it's not already set | |||
# set width and height if it's not already set | |||
# This only applies to Chart objects, compound charts like HConcatChart | |||
# don't have a 'width' or 'height' property attribute. | |||
# TODO: add support for propagating width/height options from knitr to | |||
# altair compound charts | |||
width <- py_get_attr(value, "width", TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this line (and the corresponding one for height
) are still needed, but I left them in for now.
Thank you for the PR! This seems good to me, but I'm a bit removed from altair charts and don't know how impactful or breaking this change will be for users who are currently expecting the default knitr chunk option to be respected. @cderv can you weight in? I'm inclined to merge. |
I'll go ahead and merge. @joelostblom can you please add a NEWs entry? |
Added, thank you @t-kalinowski ! |
Thank you! |
close #1642
This will avoid using the knitr default dimensions from other figure formats for altair charts. I think this makes sense since altair already has sensible and easily overidable defaults. The altair-specific kntir options are left as before and can still be used (I think it would also be an option to scrap these because as noted in the source comments, they don't work for compound charts whereas the settings in altair does).
The behavior this PR introduces is consistent with how altair works elsewhere, such as in web document and IDEs (I could not test in RStudio due to altair charts not being supported there yet as per rstudio/rstudio#8762 and rstudio/rstudio#4259). I believe this would be more intuitive for users than having the default altair dimensions overwritten by knitr (e.g. altair uses adaptive dimensions for categorical data depending on the number of categories and a consistent mark size rather than scaling the marks based on the number of categories, see example with the bar charts below).
Using the following quarto/rmarkdown source doc for testing purposes:
Quarto output BEFORE this PR
Quarto output AFTER this PR
RMarkdown output AFTER this PR
The behavior before and after is the same as for quarto so just showing one screenshot here indicating that things are working as expected.