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

Add support for rownames_to_stub + opt_interactive() #1706

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Jun 14, 2024

fixes #1702

Do you think it is worth adding a new test?

gt(iris, rownames_to_stub = T) %>% 
    opt_interactive()

Maybe the width is too wide? I don't know how to tweak it.

This PR

image

Before

image

@rich-iannone
Copy link
Member

This is great! I wouldn't worry about the width. There's a default width here that can be set for all widths in tab_options(), plus you can individually set them with cols_width(). The width rules are different in interactive tables because the underlying framework constructs a table without the <table> element, so there is no automatic sizing based on content.

@rich-iannone
Copy link
Member

Also, I wouldn't worry about the adding of a test here.

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!

@rich-iannone
Copy link
Member

I've reviewed and approved. If there is nothing more to change here, I'll merge once all tests pass.

@olivroy
Copy link
Collaborator Author

olivroy commented Jun 14, 2024

Thanks for the review. All good on my side, but let's wait for test-coverage to pass! I have tried to address #1706, but realized it was not as straight forward as I had imagined. I may take another look later on.

@rich-iannone
Copy link
Member

Thanks for taking on these bugs! If you're interested in working on #1510, it's along the same lines (but not interactive) and also a pretty serious bug. I suspect the issue there is that we are not supplying enough column width values and columns get dropped because of that.

@olivroy
Copy link
Collaborator Author

olivroy commented Jun 14, 2024

Hi @rich-iannone.

I may try again later, but here is the code I used in case it is useful to you

tb <- mtcars %>% 
  dplyr::select(mpg, cyl, vs) %>% 
  head() %>% 
  gt(groupname_col = "vs", row_group_as_column = T)

one_width <- tb %>% 
  cols_width(
    mpg ~ px(100),
  )

all_widths <-  tb %>% 
  cols_width(
    mpg ~ px(100),
    cyl ~ px(100)
  )
#' Somehow the only thing inside the gt object that is not different  
waldo::compare(
  one_width,
  all_widths
)
waldo::compare(one_width %>% render_as_html(), all_widths %>% render_as_html(), x_arg = "working", y_arg =  "bug", max_diffs = Inf, )


one_width %>% render_as_html() %>% 
  .[1] %>% 
  stringr::str_split_1(pattern = "\n") %>% 
  .[1:8]

all_widths %>% render_as_html() %>% 
  .[1] %>% 
  stringr::str_split_1(pattern = "\n") %>% 
  .[1:8]

#' # As a comparison, with `row_group_as_column = FALSE`
#' 
tb2 <-  mtcars %>% 
  dplyr::select(mpg, cyl, vs) %>% 
  head() %>% 
  gt(groupname_col = "vs", row_group_as_column = F)


one_width2 <- tb2 %>% 
  cols_width(
    mpg ~ px(100),
  )

all_widths2 <-  tb2 %>% 
  cols_width(
    mpg ~ px(100),
    cyl ~ px(100)
  )
#' Somehow the only thing inside the gt object that is not different  

# same as difference between one_width and all_widths
waldo::compare(one_width2 %>% render_as_html(), all_widths2 %>% render_as_html(), x_arg = "one_width", y_arg = "all_widths")

# same as the comparison above, so the issue is not in how gt sets col widths, the issue lies in the render html part.
waldo::compare(
  one_width2,
  all_widths2
)
waldo::compare(one_width2 %>% render_as_html(), all_widths2 %>% render_as_html(), x_arg = "one_width", y_arg = "all_widths")


# not very useful
waldo::compare(
  all_widths2 %>% render_as_html(),
  all_widths %>% render_as_html(),
  x_arg = "working",
  y_arg = "bug",
  max_diffs = Inf
)

# not very useful.
waldo::compare(
  one_width2 %>% render_as_html(),
  one_width %>% render_as_html(),
  x_arg = "working",
  y_arg = "working row_group_as_col"
)

Next steps for checking,

  • cols_width() is not the culprit here.
  • There may be a <col/> missing?
  • There are 2 consecutive ; which means that something wrong is written
  • The issue may be further than anticipated when doing the gt table

The issue may be caught more by a general html validator as locally, the rendered code doesn't tell us much...

Do you have tip on which helper function's output I should look at to really find out what is going wrong?

@rich-iannone
Copy link
Member

Thanks for looking into this one. For that issue, I think the problem is within get_table_defs(). We can continue the discussion there. As for this PR, I'll merge it now!

@rich-iannone rich-iannone merged commit af77fc1 into rstudio:master Jun 14, 2024
12 checks passed
@olivroy olivroy changed the title Add support for rowname_to_stub + opt_interactive() Add support for rownames_to_stub + opt_interactive() Jun 14, 2024
@olivroy olivroy deleted the issue-1702 branch June 19, 2024 19:37
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.

Rownames not showing when gt is set to interactive
2 participants