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 when placing flextable footnote to multiple columns and rows #2063

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ddsjoberg
Copy link
Owner

@ddsjoberg ddsjoberg commented Nov 9, 2024

What changes are proposed in this pull request?

If there is an GitHub issue associated with this pull request, please provide link.
closes #2062

@Melkiades before fully reviewing this PR, can you add a test or two more for different types of footnote placement to ensure they are correctly placed when there is one column and multiple rows, multiple columns and one row, and one mixed-type when the column/rows are contiguous? Thank you!


Reviewer Checklist (if item does not apply, mark is as complete)

  • PR branch has pulled the most recent updates from main branch.
  • If a bug was fixed, a unit test was added.
  • Run pkgdown::build_site(). Check the R console for errors, and review the rendered website.
  • Code coverage is suitable for any new functions/features: devtools::test_coverage()
  • usethis::use_spell_check() runs with no spelling errors in documentation
  • All GitHub Action workflows pass with a ✅

When the branch is ready to be merged into master:

  • Update NEWS.md with the changes from this pull request under the heading "# gtsummary (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (see NEWS.md for examples).
  • Increment the version number using usethis::use_version(which = "dev")
  • Run usethis::use_spell_check() again
  • Approve Pull Request
  • Merge the PR. Please use "Squash and merge".

@Melkiades
Copy link
Collaborator

@ddsjoberg added a test, let me know if it was what you were looking for. Only place where I cannot add a footnote is the stubhead so far

Copy link
Collaborator

@Melkiades Melkiades left a comment

Choose a reason for hiding this comment

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

Thanks Daniel for the fix. If It is what you need feel free to merge. From my side, I could not break it :D

@ddsjoberg
Copy link
Owner Author

Thanks @Melkiades ! Can you take a look into the failing CI/CD?

\(location_ids, tab_location, row_numbers) {
col_ids <- getElement(location_ids, "column_id") |> unique()
if (tab_location == "body") col_ids <- rep(col_ids, length(row_numbers)) # styler: off
sort(col_ids)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Hi @Melkiades , I was doing some final checks, and saw that we were not placing the footnote correctly. I added the sort() to get it right which changed the footnote placement from this
image

to

image

Is there a way to test that the placement of the footnote markers is where we expect in a flextable?

Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am on it!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thank you!!

@@ -303,22 +303,22 @@ test_that("as_flex_table passes multiple table footnotes correctly", {
modify_table_styling(
columns = stat_1,
rows = (variable %in% "grade") & (row_type == "level"),
footnote = "Cell-level foonotes here."
footnote = "my footnote"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Owner Author

Choose a reason for hiding this comment

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

the bug appeared when we were assigning the same footnote to multiple locations. separate footnotes were assigned with separate calls to the flextable footnote function

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see! thanks

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.

Bug Report: as_flex_table doesn't pass multi-column/row footnote coordinates correctly
2 participants