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

Expected behaviour re outer table border with nonzero gutter? #1071

Closed
mjasperse opened this issue Dec 22, 2023 · 5 comments · Fixed by #1087
Closed

Expected behaviour re outer table border with nonzero gutter? #1071

mjasperse opened this issue Dec 22, 2023 · 5 comments · Fixed by #1087

Comments

@mjasperse
Copy link

Hi guys,
Just wanted to clarify whether it is intended that the exterior border of a table is fragmented by the gutter. This seems like a bug to me, but the test suites have results committed indicating it is correct:

e.g. test_outside_border_width()
image

e.g. test_table_with_gutter_and_padding_and_outer_border_width()
image

I've changed it in my local version, but if this is the intended result I will move the change to outside the library.

Cheers,
Martijn

@Lucas-C
Copy link
Member

Lucas-C commented Dec 22, 2023

Hi @mjasperse and thank you for this report! 👍

To me this seems like a bug.
I looked at the existing test reference PDF files, and to me it would make more sense for them to have an "unbroken" outer border.

Pinging @RubendeBruin about this, as he contributed most of the padding implementation, and may have extra useful information to share regarding this 😊

But it'd be nice to see how you fixed that, so feel free to submit a PR!

@RubendeBruin
Copy link

When the outer border width is not specified then I think the border should be fragmented.

I don't think there was really a use case for the combination of outer border width and padding when I implemented it. Having it continuous would be more efficient.

@mjasperse
Copy link
Author

Before I submit a PR, I want to check what you think of the following change to handle this combination of options:
image

This adds the gutter space inside the outer border, which is compatible with how HTML renders tables with nonzero border-spacing.

@RubendeBruin
Copy link

My preference for this season would be:

image

:-)

But for the rest of the year I think your proposal is a good one.
Just one remark/question: Is the padding supposed to be to the centerline of the border or to the edge of the border? In the picture you posted it appears to be the centerline:

image

@mjasperse
Copy link
Author

@RubendeBruin thanks for the observation! I added a padding amount of outer_border_width/2 so that the spacing is more consistent as you pointed out.

andersonhc pushed a commit that referenced this issue Jan 26, 2024
* table: Changed drawing of outer border when gutter is nonzero [#1071]

* test: Regenerated tests for new handling of outer border

* table: Apply the gutter spacing between outer border and table contents [#1071]

* Updated changelog for table outer border modification

* table: Changed outer border calculation to use border edge instead of centre

* table: Draw top and bottom outer border as one line

* test: Regenerated tests for new interpretation of gutter spacing

* table: Fixed header with gutter after pagebreak

Header rows need gutter between them, as well as before the start of
content; see test_table_with_colspan()

* test: Reproduced comparison PDFs for outer border change

Outer border top and bottom lines are now single continuous lines
instead of many line segments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants