Render large tables #158

sbreitbart-NOAA · 2025-01-28T18:07:11Z

What is the feature?

Rendering large tables properly, as per Fix render with large tables #155

How have you implemented the solution?

Quantifying each table's width in create_tables_doc, then customizing the appropriate chunk in the 08_tables.qmd file so that tables wider than 5" are placed in a landscape-orientation page. Tables between 5-12" wide are forced to shrink in width to 7.5" (this shrinks the text to fit). Tables > 12" wide are split into multiple tables, saved as a new rda, and placed in individual chunks in the 08_tables.qmd doc.

Does the PR impact any other area of the project, maybe another repo?

No

github-actions · 2025-01-28T18:07:26Z

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

Please use this checklist during your review, checking off items that you have verified are complete!
For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

The PR is requested to be merged into the appropriate branch (typically main)
The code is well-designed.
The functionality is good for the users of the code.
Any User Interface changes are sensible and look good.
The code isn’t more complex than it needs to be.
Code coverage remains high, indicating the new code is tested.
The developer used clear names for everything.
Comments are clear and useful, and mostly explain why instead of what.
Code is appropriately documented (doxygen and roxygen).

Schiano-NOAA

Looks good. I have not tested but looks like in practice it should just check if the table needs to be rotated or not

Schiano-NOAA · 2025-01-28T22:51:27Z

Next thing to think about is how this could be applied for custom tables? or do we just state a recommended practice to follow?

sbreitbart-NOAA · 2025-01-29T14:42:54Z

Looks good. I have not tested but looks like in practice it should just check if the table needs to be rotated or not

Thanks! What I hope is one of the last components of this PR, which I'm working on now, is designing a way to accommodate tables that need landscape orientation but are too wide to be shrunken with the existing workflow (e.g., red snapper, which has a 60" wide indices table without shrinking, and with shrinking, has text that is too small to read).

sbreitbart-NOAA · 2025-01-29T14:46:12Z

Next thing to think about is how this could be applied for custom tables? or do we just state a recommended practice to follow?

Great point. I'll write up an explanation for our workflow and explain how the user can do it manually for custom tables in the FAQs and manual.

… to require changing page orientation from portrait to landscape. Alter the Tables qmd to allow orientation changes for each table, if necessary.

…n tables

…o smaller ones

…y splitting into multiple tables

…te create_tables_doc

…date documentation and usage in create_tables_doc()

…multiple smaller tables, but still a bug in render_lg_table (producing "Error" as part of output when testing)

…ccessfully rendering extra-wide table with new solution, but need to check if this will work in other circumstances

…p: apply to other tables, and simplify/clarify the new functions

…e tables will only reference the last table in the chunk, not the first. So next step: make a chunk for each table. Also removed now irrelevant function: ID_split_tbls

… can be referenced.

…s is made; update documentation

Schiano-NOAA

I reran the create_tables_doc function from a previous example and it worked...however, it repeated the table caption over and over in the list of tables in the beginning of the doc and broke it up incorrectly in the document itself (I think). Guess we will need to chat about this next week. I can retest on monday with a fresh mind just in case :)

sbreitbart-NOAA · 2025-02-10T13:58:26Z

I reran the create_tables_doc function from a previous example and it worked...however, it repeated the table caption over and over in the list of tables in the beginning of the doc and broke it up incorrectly in the document itself (I think). Guess we will need to chat about this next week. I can retest on monday with a fresh mind just in case :)

Thanks for testing. Can you please explain how it's breaking the tables up incorrectly? And what would you prefer the table captions look like?

Schiano-NOAA · 2025-02-10T14:03:15Z

I reran the create_tables_doc function from a previous example and it worked...however, it repeated the table caption over and over in the list of tables in the beginning of the doc and broke it up incorrectly in the document itself (I think). Guess we will need to chat about this next week. I can retest on monday with a fresh mind just in case :)

Thanks for testing. Can you please explain how it's breaking the tables up incorrectly? And what would you prefer the table captions look like?

Schiano-NOAA · 2025-02-10T14:04:44Z

@sbreitbart-NOAA I think the tables are broken up fine but I wonder if it's possible to add into the caption the fleets that are included in the separated table

…in parentheses after caption (example: <indices caption> (Fleet 6, Fleet 7))

sbreitbart-NOAA · 2025-02-10T21:55:39Z

@Schiano-NOAA I've updated the workflow so that when a table is split, the header values for each table are added in parentheses to the end of the table caption, like this:

Is this change sufficient to approve the PR? Or should I make more changes to the workflow?

Schiano-NOAA · 2025-02-10T22:03:42Z

@Schiano-NOAA I've updated the workflow so that when a table is split, the header values for each table are added in parentheses to the end of the table caption, like this:

Is this change sufficient to approve the PR? Or should I make more changes to the workflow?

This should be good. Let me do a quick test

sbreitbart-NOAA linked an issue Jan 28, 2025 that may be closed by this pull request

Fix render with large tables #155

Closed

Schiano-NOAA reviewed Jan 28, 2025

View reviewed changes

Schiano-NOAA force-pushed the dev branch 2 times, most recently from 899f3fb to 7439a1c Compare February 6, 2025 14:40

sbreitbart-NOAA force-pushed the render-lg-tbls branch from 06181fd to 1f2d01f Compare February 6, 2025 16:54

sbreitbart-NOAA self-assigned this Feb 6, 2025

sbreitbart-NOAA added 20 commits February 6, 2025 16:56

Feat(create_tables_doc): automatically test if tables are wide enough…

c7825e1

… to require changing page orientation from portrait to landscape. Alter the Tables qmd to allow orientation changes for each table, if necessary.

Update orientation-related code for still-in-dev landings table

2efa7d0

Extend landscape table width from 7.5 to 8.5"; add page breaks betwee…

ad0c006

…n tables

bug fixes

9e580d1

Started coding new function to handle super wide tables and split int…

c578503

…o smaller ones

Finished first full draft of function that will render large tables b…

308dff3

…y splitting into multiple tables

Added essential_columns arg to render_lg_table()

66ba87c

Move orientation-detecting code into own function in utils, then upda…

942ea95

…te create_tables_doc

Moved render_lg_table fxn to own R script, then updated documentation

737ccd0

Update tests to remove temporary files upon test completion

089b286

Add more params to render_lg_table() and ID_pg_orientation(), then up…

6ebb754

…date documentation and usage in create_tables_doc()

Added code into create_tables_doc that splits extra-wide tables into …

5eba27e

…multiple smaller tables, but still a bug in render_lg_table (producing "Error" as part of output when testing)

Found a solution for sourcing render_lg_table in 08_tables.qmd and su…

6b6be00

…ccessfully rendering extra-wide table with new solution, but need to check if this will work in other circumstances

found a successful workflow and applied it to indices table. Next ste…

112fd58

…p: apply to other tables, and simplify/clarify the new functions

Copied new changes to bnc table, but referencing a chunk with multipl…

bce8890

…e tables will only reference the last table in the chunk, not the first. So next step: make a chunk for each table. Also removed now irrelevant function: ID_split_tbls

Redesigned create_tables_doc so that each table has its own chunk and…

30c7e1a

… can be referenced.

Updated landings table code

01836e1

Removed "-plot" from create_figures/tables_doc chunks

0f5e522

Add note to create_tables_doc explaining why new rda with split table…

e7ee67d

…s is made; update documentation

First draft of render_lg_table test; update documentation

625a669

sbreitbart-NOAA force-pushed the render-lg-tbls branch from 1f2d01f to 625a669 Compare February 6, 2025 21:59

Remove code from create_tables_doc that reduces row height in tables

7174f53

sbreitbart-NOAA marked this pull request as ready for review February 6, 2025 22:26

sbreitbart-NOAA requested a review from Schiano-NOAA February 6, 2025 22:27

Schiano-NOAA requested changes Feb 7, 2025

View reviewed changes

Updated create_tables_doc so that split tables include header values …

0a28fca

…in parentheses after caption (example: <indices caption> (Fleet 6, Fleet 7))

Updated examples

7b803e2

sbreitbart-NOAA merged commit 6b97ddc into dev Feb 11, 2025

sbreitbart-NOAA deleted the render-lg-tbls branch February 11, 2025 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render large tables #158

Render large tables #158

sbreitbart-NOAA commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited by sbreitbart-NOAA

Loading

Schiano-NOAA left a comment

Schiano-NOAA commented Jan 28, 2025

sbreitbart-NOAA commented Jan 29, 2025

sbreitbart-NOAA commented Jan 29, 2025

Schiano-NOAA left a comment

sbreitbart-NOAA commented Feb 10, 2025

Schiano-NOAA commented Feb 10, 2025

Schiano-NOAA commented Feb 10, 2025

sbreitbart-NOAA commented Feb 10, 2025

Schiano-NOAA commented Feb 10, 2025

Render large tables #158

Render large tables #158

Conversation

sbreitbart-NOAA commented Jan 28, 2025 • edited Loading

What is the feature?

How have you implemented the solution?

Does the PR impact any other area of the project, maybe another repo?

github-actions bot commented Jan 28, 2025 • edited by sbreitbart-NOAA Loading

Instructions for code reviewer

Checklist

Schiano-NOAA left a comment

Choose a reason for hiding this comment

Schiano-NOAA commented Jan 28, 2025

sbreitbart-NOAA commented Jan 29, 2025

sbreitbart-NOAA commented Jan 29, 2025

Schiano-NOAA left a comment

Choose a reason for hiding this comment

sbreitbart-NOAA commented Feb 10, 2025

Schiano-NOAA commented Feb 10, 2025

Schiano-NOAA commented Feb 10, 2025

sbreitbart-NOAA commented Feb 10, 2025

Schiano-NOAA commented Feb 10, 2025

sbreitbart-NOAA commented Jan 28, 2025 •

edited

Loading

github-actions bot commented Jan 28, 2025 •

edited by sbreitbart-NOAA

Loading