Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
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

Support adding additional table headers #293

Merged
merged 9 commits into from
Dec 6, 2022
Merged

Support adding additional table headers #293

merged 9 commits into from
Dec 6, 2022

Conversation

KindaOK
Copy link
Contributor

@KindaOK KindaOK commented Nov 24, 2022

Resolves #272

Adds the option to create multiple table headers provided that no rows have been created yet.

Currently defers header rendering until the first row is placed. This does change existing behavior in that respect.

Passes all tests locally.

Implements Feature Request from #272

Headers are now stored as an array and operations have been changed to work with it. Rendering the header has been deferred to row creation, which is not ideal and should be adjusted.
Multiple table headers are now allowed, but they cannot be mixed with table rows.
May be exhibiting buggy behavior per #292.
Copy link
Owner

@rkusa rkusa left a comment

Choose a reason for hiding this comment

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

Well done, kudos for digging through this rather old code 👍. I think there is only one thing left (see inline comment).

lib/table.js Show resolved Hide resolved
Previously, header rendering was deferred until row creation. This is changed so tables can now be exclusively headers. Border behavior is also adjusted for header-only tables to ensure that bottom borders will be created for the last header row.
Test related to issue #292. The test isn't good as a basic test, but is worth keeping around.
Headers should render and have a bottom border if borders are included.
Table header rows would skip an index when a table with exclusively headers is rendered.
A simple test to showcase the use of multiple headers and how it looks spread out across multiple pages.
@KindaOK
Copy link
Contributor Author

KindaOK commented Nov 29, 2022

Fixed the issue. Also added some code to render the bottom border of a header along with unit tests.

Copy link
Owner

@rkusa rkusa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again and I especially appreciate the various test cases 👍

@rkusa rkusa merged commit c7de33c into rkusa:main Dec 6, 2022
@rkusa
Copy link
Owner

rkusa commented Dec 6, 2022

While preparing a release I've just noticed the following warnings in the simple-multiple-headers test:

# table/simple-multiple-headers
set cursor.y to negative value
set cursor.y to negative value
set cursor.y to negative value
set cursor.y to negative value
set cursor.y to negative value
set cursor.y to negative value
ok 116 simple-multiple-headers

Any idea what could cause that? If not, don't worry about it, I can also look into it

@KindaOK
Copy link
Contributor Author

KindaOK commented Dec 6, 2022

It has to do with the size of the headers and fitting them on a page. Some experimentation on the test is below.

Num Headers (i < x) Vertical Padding Warning Messages
3 10 None
3 20 6
3 30 9
3 40 None
4 20 3
8 10 None
9 10 9
10 10 None

Error is reached here.

this._cursor.y -= this.paddingTop

@KindaOK KindaOK deleted the multiple-headers branch December 6, 2022 20:49
rkusa added a commit that referenced this pull request Jan 11, 2023
@rkusa
Copy link
Owner

rkusa commented Jan 11, 2023

I removed the warning altogether. It was also logged in other tests and there are cases where a negative y isn't an issue (as content is rendered to a "content chunk" that is later placed e.g. on the next page with another total vertical offset).

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.

Error: The table already has a header, add additional rows to the existing table header instead
2 participants