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

gt table support #54

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft

Conversation

yjunechoe
Copy link

@yjunechoe yjunechoe commented Jul 30, 2023

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 where skip_*() 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!

@gkaramanis
Copy link
Collaborator

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?

@yjunechoe
Copy link
Author

yjunechoe commented Aug 1, 2023

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:

  1. Expose the vwidth/vheight that {webshot2} uses for viewport width/height. This is roughly the amount of room in the html doc that the table gets when it's rendered. This controls stuff like whether a long column gets wrapped, but is agnostic to the size of the table when converted to image. This is faithful to how width/height is understood in {webshot2}
  2. The existing width/height argument should be reserved for image dimensions. {webshot2} doesn't control the dimensions of the screenshot, but we could add a post-processing step re-sizing the table image with {magick}, to make table dims match plot dims.

So then record_ggplot() for gt tables would roughly look something like:

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(), you can set both width/height and (if recording gt) vwidth/vheight:

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

@thebioengineer
Copy link
Owner

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 .

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.

3 participants