insightsengineering / formatters

A framework for creating listings of raw data that include specialized formatting, headers, footers, referential footnotes, and pagination.
https://insightsengineering.github.io/formatters/
Other
15 stars 6 forks source link

Fix horizontal pagination to not keep into account trailing `col_gap` when finding pagination points #249

Closed Melkiades closed 4 months ago

Melkiades commented 9 months ago

See tests that are followed by this comment in {rlistings}

This needs to be solved in formatters. It may not be only {rlistings}

_Originally posted by @Melkiades in https://github.com/insightsengineering/rlistings/pull/192#discussion_r1458817388_

gmbecker commented 8 months ago

@Melkiades Assuming you mean that horizontal pagination should take into account how large the column gap is declared to be (and allowing the user to declare how big it should be in all the relevant places), this is included in the large incoming PR related to pagination I've been developing

Melkiades commented 8 months ago

@Melkiades Assuming you mean that horizontal pagination should take into account how large the column gap is declared to be (and allowing the user to declare how big it should be in all the relevant places), this is included in the large incoming PR related to pagination I've been developing

There are cases as the one linked above in which there is an extra col_gap at the end of the line that is not needed for pagination:

# manual calculation of the cpp
expected_min_cpp <- sum(cw[seq_len(3)]) + 2 * colgap
pgs <- paginate_to_mpfs(lst, colwidths = cw, lpp = 150, cpp = expected_min_cpp + 3) # why + 3? -> + colgap

If no +3 is provided there, it will fail

gmbecker commented 8 months ago

The issue there is rlistings-specific, or more accurately, its specific to anything that doesn't have row labels (which currently is only rlistings). When row labels are present, the all columns take up width + colgap in actual real estate (ie the colgaps are preceeding). When there are no row labels, the first column doesn't need a preceeding colgap, but it looks like thats not being handled correctly. Thats where the +3 requirement is coming from.

I will make sure that my PR works for the no row labels case, since it had to touch the colgap machinery anyway to get other things to work. I'd prefer not to have other conflicting changes happening at the same time, if possible, since this issue has been open a month and my PR will be submitted sometime this week if things go according to plan, and should fix it.