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

Fix table widths for latex tables and add unit testing #1867

Merged
merged 8 commits into from
Oct 2, 2024

Conversation

snhansen
Copy link
Contributor

@snhansen snhansen commented Aug 30, 2024

Summary

In the old code, col_alignment contains the aligment of stub columns and visible columns, in that order. In the loop for (i in seq_along(col_alignment)), the ith row of colwidth_df is referenced. However, there are multiple problems with that:

  • colwidth_df contains hidden columns, so the ith row of that data frame doesn't not necessarily correspond to the ith item of col_alignment.
  • stub columns is not necessarily at the beginning of colwidth_df

In this PR,

  • we change the function create_colwidth_df_l() to include column alignments.
  • we rewrite create_table_start_l() to extract only the visible columns of colwidth_df and we make sure the order of the rows in colwidth_df_visible is "row_group", "stub", "default". The loop is then taken over the rows of colwidth_df_visible.
  • we add some unit testing in test-l_cols_width.R. We test five different tables where we test whether col_widths() works correctly in different scenarios, e.g. when merging columns, when using stubs with/without row groups as a column.

Screenshots

Below are some before and after screenshots of the five examples in test-l_cols_width.R with latex.use_longtable being TRUE and FALSE in that order. When not using longtable, I've set the table.width manually in the screenshots, otherwise the resulting table will be a full-width table from which the column widths can't really be judged.

Table 1

image

Table 2

image

Table 3

image

Table 4

image
image

Table 5

image

Additional comments:

Related GitHub Issues and PRs

Checklist

Add tests for setting table widths for latex tables
@CLAassistant
Copy link

CLAassistant commented Aug 30, 2024

CLA assistant check
All committers have signed the CLA.

@olivroy
Copy link
Collaborator

olivroy commented Aug 30, 2024

Hi @snhansen! Thanks for working on this! We will try to review soon. But first, please use %>% as we still support R 3.6! |> was introduced in R 4.1! Thanks

@snhansen
Copy link
Contributor Author

@olivroy: Of course! Should be done now.

@olivroy
Copy link
Collaborator

olivroy commented Aug 30, 2024

I didn't notice before, but it seems like your changes affect other tests https://github.com/rstudio/gt/actions/runs/10629794644/job/29475689750. You'd have to review and accept those if they make sense!

@snhansen
Copy link
Contributor Author

snhansen commented Aug 30, 2024

@olivroy: Thanks, the tests did indeed reveal a mistake on my part. I think it should be good now though. Could you press the button to run tests again? 🤞

@olivroy
Copy link
Collaborator

olivroy commented Aug 30, 2024

I unfortunately don't have that power, but we will review soon! from what I can see, it looks pretty good. The testing seems thorough and the explanations are clear. (All tests also pass locally for me)

Stub columns are automatically left-aligned for LaTeX output. This is so for the code in this PR as well as the old code.

If you wish to change that, feel free to open a new PR!

It might be an idea not to go full-width for tables using tabular if all column widths are specified (#1857).

Agreed. Full widths tables were never a thing before in gt. See this comment about that #1588 (comment)

Tables using tabular are left-aligned by default. Are we able to control this? (#1848)

It would be great if there was a Quarto div for this. quarto-dev/quarto-cli#8705 (comment) is a solution.

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.

Looks great, thanks!

@rich-iannone rich-iannone merged commit 72664c3 into rstudio:master Oct 2, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants