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

Improve access to option values + use htmltools to create some css code #1868

Merged
merged 11 commits into from
Sep 3, 2024

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Aug 30, 2024

Summary

  • Use less dplyr in footnote code
  • Use htmltools to generate some html strings
  • Move fmt_time(), fmt_datetime() and fmt_date() in own file
  • Lints to tests
  • Create a helper to get all tab_options() values to facilitate reusing them.

Related GitHub Issues and PRs

Before
image
This PR
image

Checklist

@@ -309,14 +292,23 @@ get_table_defs <- function(data) {
if (table_width == "auto") {

if (all(grepl("px", widths, fixed = TRUE))) {
# FIXME sometimes ends up being 0? #1532 and quarto-dev/quarto-cli#8233
table_width <- "0px"
} else if (all(grepl("%", widths, fixed = TRUE))) {
table_width <- "100%"
}
}

if (table_width != "auto") {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rich-iannone to fix #1532, should we just have this condition be

Suggested change
if (table_width != "auto") {
if (!table_width %in% c("auto", "0", "0px")) {

If so, I will create a PR after this one is merged and update all snapshots.

I did the refactoring here to make the potential fix clearer in the future

Copy link
Member

Choose a reason for hiding this comment

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

I like the version that catches the zero-width table! I’d welcome a future PR that addresses that problem.

filtered_tbl <- dplyr::filter(fn_tbl, locname == !!locname)
dplyr::summarize(filtered_tbl, fs_id_c = paste(fs_id, collapse = delimiter))
fs_ids <- vctrs::vec_slice(fn_tbl$fs_id, fn_tbl$locname == locname)
paste(fs_ids, collapse = delimiter)
Copy link
Collaborator Author

@olivroy olivroy Aug 30, 2024

Choose a reason for hiding this comment

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

Note that I refactored coalesce_marks() to return a single string because the resulting data frame (1 col x 1 row) previously was never used, and all code needed to access the fs_id_c variable. e86b842

Copy link
Member

Choose a reason for hiding this comment

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

This is a great improvement! Yeah, a vector is all we need.

Comment on lines +61 to +68
#' @export
`$.gt_option` <- function(x, name) {
out <- .subset2(x, name)
if (is.null(out)) {
cli::cli_abort("Can't find option {.val {name}}.")
}
out
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is to avoid a typo when developing dt_options_get_values(data)$olumn_label errors instead of returning NULL, similar to the method that tibble defines for tables. I have been bitten while developing, so I thought it was better to catch the error on the spot for easier debugging

exibble$num2

#> Warning: Unknown or uninitalized column
mtcars$nonexistent
#> NULL

Copy link
Member

Choose a reason for hiding this comment

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

I like it!

@olivroy olivroy changed the title Some misc refactoring Improve access to option values + use htmltools to create some css code Aug 30, 2024
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 rich-iannone merged commit 340f5d7 into rstudio:master Sep 3, 2024
12 checks passed
@olivroy olivroy deleted the refactor-misc branch September 3, 2024 16:58
rich-iannone added a commit that referenced this pull request Sep 3, 2024
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.

2 participants