-
Notifications
You must be signed in to change notification settings - Fork 142
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
Conversation
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.
There was a problem hiding this 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).
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.
Fixed the issue. Also added some code to render the bottom border of a header along with unit tests. |
There was a problem hiding this 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 👍
While preparing a release I've just noticed the following warnings in the
Any idea what could cause that? If not, don't worry about it, I can also look into it |
It has to do with the size of the headers and fitting them on a page. Some experimentation on the test is below.
Error is reached here. Line 248 in 55a662f
|
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). |
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.