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.

Sign up for GitHub

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

Jump to bottom

Allowing to set padding in tables and multi_cell + vertical alignment in tables #797

Merged
merged 103 commits into from
Sep 23, 2023
Merged

Allowing to set padding in tables and multi_cell + vertical alignment in tables #797

merged 103 commits into from
Sep 23, 2023

Conversation

RubendeBruin
Copy link

@RubendeBruin RubendeBruin commented Jun 1, 2023 •
edited
Loading

This PR adds an optional "padding" argument to multi_cell()

Using the same definition as in CSS:
"padding (float or Sequence): padding to apply around the text. Default value: 0.
When one value is specified, it applies the same padding to all four sides.
When two values are specified, the first padding applies to the top and bottom, the second to the left and right.
When three values are specified, the first padding applies to the top, the second to the right and left, the third to the bottom.
When four values are specified, the paddings apply to the top, right, bottom, and left in that order (clockwise)"

Fixes #791 #892 #893

Checklist:

  • The GitHub pipeline is OK (green),
    meaning that both pylint (static code analyzer) and black (code formatter) are happy with the changes of this PR.

  • A unit test is covering the code added / modified by this PR

  • This PR is ready to be merged

  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

Progress:

  • Added padding to multi_cell
  • Added unit check for multi_cell
  • Implemented padding for tables on cell level
  • add unit check
  • Implement for images as well
  • Fix image not creating a page-break
  • Add v_align option

@RubendeBruin RubendeBruin marked this pull request as draft June 1, 2023 19:19
@RubendeBruin RubendeBruin marked this pull request as ready for review June 2, 2023 09:18
@RubendeBruin RubendeBruin changed the title WIP : Padding in tables and multi_cell Padding in tables and multi_cell Jun 2, 2023
@RubendeBruin
Copy link
Author

RubendeBruin commented Jun 2, 2023 •
edited
Loading

Seems to be working so far.
image

Tests:

Previously the cell borders were drawn per text-line. In this PR the borders are drawn for the whole cell at once and before rendering the text. The result is the same but the pdf structure is different which makes the tests fail. I guess I need to manually check if the produces pdf looks the same and then update the reference pdf?

Another thing is that I would like to set the default value for padding to half the text-height. This would however cause all the previous unit-tests to fail.

@RubendeBruin
Copy link
Author

RubendeBruin commented Jun 2, 2023 •
edited
Loading

Vertical alignment added:

Original:
image

This PR:
image

@RubendeBruin
Copy link
Author

Ok, time for weekend.
All features are implemented. Documentation is added to Tables.md

Unit tests have been written but only open the pdf for review. Will fix that once everybody is happy. Same for change-log

@Lucas-C Lucas-C changed the title Padding in tables and multi_cell Allowing to control padding of tables and multi_cell + vertical alignment in tables Jun 5, 2023
@Lucas-C Lucas-C mentioned this pull request Jun 5, 2023
Closed
Lucas-C
Lucas-C reviewed Jun 5, 2023
View reviewed changes
fpdf/fpdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Jun 5, 2023
View reviewed changes
fpdf/fpdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Jun 5, 2023
View reviewed changes
fpdf/fpdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Jun 5, 2023
View reviewed changes
fpdf/fpdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Jun 5, 2023
View reviewed changes
fpdf/fpdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Jun 5, 2023
View reviewed changes
fpdf/fpdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Jun 5, 2023
View reviewed changes
fpdf/fpdf.py Outdated Show resolved Hide resolved
Lucas-C
Lucas-C reviewed Jun 5, 2023
View reviewed changes
fpdf/fpdf.py Outdated Show resolved Hide resolved
@RubendeBruin
Copy link
Author

Hi @Lucas-C , I've re-requested a review because the workflow:
image
keeps complaining that there is still an open change request. I can not find it. I think I took care of all the requests, either by implementing them or commenting on why they are not implemented.

@RubendeBruin
Copy link
Author

Ready to be merged again.

@gmischler
Copy link
Collaborator

On my branch that pdf look like:

I assume the correct one is your local copy?

The copy on your Github fork branch is the weird version. Looks like something is out of sync there. Maybe you should try to regenerate your local copy with a fresh timestamp, so hopefully the correct version gets pushed to the server with the next commit.

@RubendeBruin
Copy link
Author

On my branch that pdf look like:

I assume the correct one is your local copy?

The copy on your Github fork branch is the weird version. Looks like something is out of sync there. Maybe you should try to regenerate your local copy with a fresh timestamp, so hopefully the correct version gets pushed to the server with the next commit.

That is strange. How can the tests pass with the wrong reference pdf???

Ah:
image

there is no dedicated test for it. It became part of table_colspan_and_padding_and_gutter.

@RubendeBruin
Copy link
Author

I removed table_colspan_and_gutter.pdf locally and pushed. This removed the file on github as well. Remains a small mystery why they were out of sync in the first place...

Ready to be merged

gmischler
gmischler requested changes Sep 14, 2023
View reviewed changes
fpdf/enums.py Outdated Show resolved Hide resolved
@RubendeBruin
Replaced "Center" with "Middle" for vertical alignment in tables to b…
66cdd7e 
…e consistent with CSS.

(updated one reference PDF because the vertical alignment was printed there as text)
@RubendeBruin
And updated the default value as well
6607dc3
@gmischler
Copy link
Collaborator

Another rebase, and it's in! 👍

@RubendeBruin
Merge remote-tracking branch 'origin/master' into padding
01f8909 
# Conflicts:
#	CHANGELOG.md
#	test/html/html_table_th_inside_tr_issue_137.pdf
#	test/html/html_table_with_border.pdf
#	test/html/html_table_with_multi_lines_text.pdf
@RubendeBruin
And updated the default value as well
ac069a3
@RubendeBruin
Re-generated the pdfs of HTML tests with tables
1ffc9a3
@RubendeBruin
Copy link
Author

Tests fail due to tiemout only.
https://github.com/py-pdf/fpdf2/actions/runs/6272521714/job/17034201615?pr=797#step:10:1594

Ready to be merged.

gmischler
gmischler reviewed Sep 22, 2023
View reviewed changes
docs/Tables.md Outdated Show resolved Hide resolved
@RubendeBruin
Update Tables.md
930cdae 
version number for padding 2.7.5 --> 2.7.6
@gmischler
Copy link
Collaborator

It looks to be working nicely, passes all the tests, and you seem to have addressed all of @Lucas-C s concerns and suggestions.
So I'm going to ignore that he didn't formally approve yet and send it!

@gmischler gmischler merged commit 7f4e0a3 into py-pdf:master Sep 23, 2023
10 checks passed
@Lucas-C
Copy link
Member

Lucas-C commented Sep 27, 2023 •
edited
Loading

Thank you for reviewing & merging this @gmischler,
and above all thank you @RubendeBruin for this excellent contribution! 👍👍👍

Lucas-C
Lucas-C reviewed Sep 27, 2023
View reviewed changes
CHANGELOG.md
### Fixed
* [`FPDF.image()`](https://py-pdf.github.io/fpdf2/fpdf/fpdf.html#fpdf.fpdf.FPDF.image), when provided a `BytesIO` instance, does not close it anymore - _cf._ issue [#881](https://github.com/py-pdf/fpdf2/issues/881)
* Invalid characters were being generated when a string contains parentheses - _cf._ issue [#884](https://github.com/py-pdf/fpdf2/issues/884)
* Frozen Glyph dataclass was causing problems for FPDFRecorder with TTF fonts - _cf._ issue [#890](https://github.com/py-pdf/fpdf2/issues/890)
* Edge case when parsing a Markdown link followed by a newline - _cf._ issue [#916](https://github.com/py-pdf/fpdf2/issues/916), and when bold/italics/underline markers are
* Zoom not set correctly when a numeric value was set in [`set_display_mode()`](https://py-pdf.github.io/fpdf2/fpdf/fpdf.html#fpdf.fpdf.FPDF.set_display_mode) - _cf._ issue [#926](https://github.com/py-pdf/fpdf2/issues/926)
Copy link
Member

@Lucas-C Lucas-C Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think a CHANGELOG.md entry was deleted there, and also on line 23 😅

No worries, I will restore them

@Lucas-C Lucas-C mentioned this pull request Nov 20, 2023
Closed
@Lucas-C Lucas-C mentioned this pull request Feb 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lucas-C Lucas-C Lucas-C requested changes

@gmischler gmischler gmischler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature request: set padding in a table cell
6 participants
@RubendeBruin @Lucas-C @Rahulscan @codecov-commenter @gmischler @andersonhc