-
Notifications
You must be signed in to change notification settings - Fork 7
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
gt table support #54
base: main
Are you sure you want to change the base?
gt table support #54
Conversation
Hi June! This is so great! Did some quick testing and it works flawlessly. I'd love to have support for {gt} tables in {camcorder}, let's see what @thebioengineer and @z3tt think 😄 One thing I'm wondering about is the dimensions in camcorder. Setting the height and width in this case doesn't really work as when saving plots (and it probably shouldn't, anyway). I guess it's a webshot thing, I remember it being tricky with dimensions when I used it before. What I'm trying to say is that, if we add {gt} support, we should add a warning about the dimensions? |
Thanks @gkaramanis! You're totally right about the different behavior of height/width for gt tables - I hadn't thought it through that far so good catch 😅 As you say it's tricky and not a great way out of this, but perhaps we could doing something like:
So then function(x) {
gt_html <- gt::gtsave(x)
gt_screenshot <- webshot2::webshot(gt_html, vwidth, vheight, ...)
gt_image <- magick::image_resize(gt_screenshot, width, height, ...)
...
} And in gg_record(
...,
# shared options
width,
height,
...
# gt-specific options
vwidth,
vheight,
...
) Whatever we decide, I definitely agree that setting dimensions for tables in {camcorder} could use a warning or note! I'll also play around with handling gt dimensions a bit more and report back what I find |
Hey @yjunechoe - this is a great idea, and I would be happy to expand the scope of camcorder to include tables. In theory this could expand to other html-based views, but we can sort that out later. I would prefer to not add a lot of new arguments to gg_record specific to supporting gt tables, but if we could sort out another way. The height and width expected by webshot2 in pixels, so we can always do unit conversion from what is passed in into viewer height. I think the usual assumption is 96 pixels per inch . |
Hey all,
I got nerd-sniped on masstodon by a {camcorder} user who wanted to see support for {gt} tables. This draft PR attempts to add that, with some examples you can view in
_snaps/gt
.Now that I have a working prototype, I wanted to gauge whether this feature would be of interest 😃 (and possible get some more hands on board for testing).
A list of things to consider:
Scope: obviously adding {gt} support changes the scope of the package a bit. Now it's not just plots but also tables. IMO this is actually not too far off from the broader goals of the {camcorder} - now the exploratory data wrangling can be part of the playback too. Documentation should probably(?) get a small edit to refer generally to "images" vs. specifically to "plots".
Dependencies: other than {gt}, the PR also adds {webshot2} which is used to convert html to png. These are under Suggests.
Tests: they're in
test-gt.R
and probably need a second look, especially whereskip_*()
is concerned (wasn't entirely sure what kinds of tests you wanted to skip when). BTW on my machine the boxplot tests are failing because the linejoin is round (whereas the default is mitre) - I left them untouched for now.Known issues:
gt table dimensions are tricky to work with (need extra work to ensure compatibility with the behavior of width/height for recording plots)
webshot2 "screenshot completed" message can't be suppressed
LMK what you think!