Skip to content

Improvements to grid context as_gtable() + plot.gt_tbl() #1788

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

Merged
merged 10 commits into from
Jul 15, 2024

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Jul 15, 2024

Summary

@teunbrand I'd love to have your views here in general.

  • Does this seem reasonable?
  • Should we use vdiffr to capture the tables as a useful visual reference?
  • I am not too sure of what I did here. My testing is mostly through trial and error.
  • Any concerns with unicode?

Related GitHub Issues and PRs

@rich-iannone I am not too sure if I am duplicating things here. Not clear to me how context = "plain" vs context ="grid" should act.

It is just that grid renders unicode formats, since R 4.2 I think?

Do you think we may add some conditions on R version for some grid features? i.e. if context = "grid" and R version < 4.2, use plain text rendering?

Examples

This PR resolves the following to look the same in html and grid

# this works with only a few warnings!
info_currencies() |> plot()
gt_tbl <- exibble |> 
  gt() |> 
  cols_label(
    num = md("num<br>x")
  )

plot(gt_tbl)

Previously, we would see <br>

@olivroy olivroy changed the title Improvements to grid context Improvements to grid context as_gtable() + plot.gt_tbl() Jul 15, 2024
@rich-iannone
Copy link
Member

@olivroy

I don't think we should limit grid table output to those using R 4.2 or later in practice (unless there are errors). The better thing to do might be to include some notes about this in the new FAQ section (great addition btw!). The topic could generally outline the capabilities/problems of the different table output types.

The context of "grid" seems good to me instead of using "plain" (and we ought to better define what that really is, my estimate right now is ASCII formatting outputs). We should have another context like "utf8" (or similar) for formatting outputs that can include characters outside the ASCII range.

One more thing, we ought to think of visual tests or just additional Quarto websites for grid-table outputs (and also for LaTeX, that's probably more important). I'll try working on this a bit (the Quarto website for grid-table examples) and I think it just involves adding a plot statement to certain example statements that produce a gt table.

Copy link
Member

@rich-iannone rich-iannone left a comment

Choose a reason for hiding this comment

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

LGTM!

@teunbrand
Copy link
Contributor

Does this seem reasonable?

It doesn't look unreasonable, but I don't really know what markdown::mark() or commonmark::markdown_text() are doing, so feel free to dismiss.

Should we use vdiffr to capture the tables as a useful visual reference?

You can, but it is an extra suggest dependency that I'm not sure pays off here. For testing purposes the layout object created in the line below should have most of the relevant components (i.e. labels/layout) to test:

gt/R/export.R

Line 1194 in 34fd17f

layout <-

Any concerns with unicode?

No grid should render unicode just fine even before R 4.2.0 I think so this shouldn't be cause for worry. Potentially it could be the case that a unicode glyph is not in the font, so it might render as missing characters in devices that don't have font fallbacks. But that's just a limitation, so there is nothing that {gt} can do here.

@olivroy
Copy link
Collaborator Author

olivroy commented Jul 15, 2024

Maybe I can provide an explanation here. It feels a bit magical, but since the currencies are stored in html strings, I discovered that markdown::mark(gt:::currencies$symbol, format = "text") transformed the html codes to their utf-8 equivalent. (This allows us showing the currency symbol instead of their unescaped equivalent) GBP for British pounds for example.

@rich-iannone rich-iannone merged commit c737a0b into rstudio:master Jul 15, 2024
12 checks passed
@olivroy olivroy deleted the grid-improve branch July 15, 2024 20:33
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