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

New Line After Header *Before* Font Change #261

Closed
wants to merge 1 commit into from

Conversation

jkfitzsimons
Copy link

Currently, when exiting the h1/h2/etc tag the process was to first pop to the previous font and then create a new line (pdf.ln(self.h)). The problem is that if the head font is large and the previous font was significantly smaller, then the new line is not large enough to escape past the bottom of the head font. I've simple reversed this order and tested it locally - seems to work perfectly now for various header and non-header font sizes.

Currently, when exiting the h1/h2/etc tag the process was to first pop to the previous font and _then_ create a new line (`pdf.ln(self.h)`). The problem is that if the head font is large and the previous font was significantly smaller, then the new line is not large enough to escape past the bottom of the head font. I've simple reversed this order and tested it locally - seems to work perfectly now for various header and non-header font sizes.
@Lucas-C
Copy link
Member

Lucas-C commented Oct 15, 2021

Thanks for reporting this issue.

Sadly this seems to cause tests to fail...

FAILED test/html/test_html.py::test_html_features
FAILED test/html/test_html.py::test_html_headings_line_height
FAILED test/html/test_html.py::test_html_custom_heading_sizes
FAILED test/outline/test_outline_html.py::test_html_toc
FAILED test/outline/test_outline_html.py::test_html_toc_2_pages
FAILED test/outline/test_outline_html.py::test_html_toc_with_h1_as_2nd_heading
FAILED test/outline/test_outline_html.py::test_custom_HTML2FPDF

I haven't looked into it much, I only checked the CI pipeline logs.
You may want to run those tests locally on your side to figure if this PR causes any visual difference,
or if this is purely an imperceptible PDF binary change.

Also @jkfitzsimons: could you please provide some minimal code reproducing your issue,
so that we can add a unit test for it?

@jkfitzsimons
Copy link
Author

Got it - I'll run the tests locally and share the comparison. I think it's just the assert pdf equal commands in the test will be slightly different, hence causing the CI fail.

@Lucas-C
Copy link
Member

Lucas-C commented Oct 29, 2021

Hi @jkfitzsimons !

I haven't asked so far: would you like this PR be merged as part of hacktoberfest?
I will be less available this week-end to review & merge PRs, but I'll try to check things here on Sunday.

@jkfitzsimons
Copy link
Author

Oh not at all, I only pushed it as it caused a problem for me and I figured may as well solve it for others too. Sorry for the delay in running the tests, I've been swamped with work but hoping to catch a breather shortly :)

@Lucas-C
Copy link
Member

Lucas-C commented Oct 29, 2021

Ok, no worries then 😉
And good luck with work!

@Lucas-C
Copy link
Member

Lucas-C commented Feb 7, 2022

Hi @jkfitzsimons !

Could you explain what issue this PR aims to fix?

Given this PR hasn't seen any activity over the past months, I may consider closing it...

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.

2 participants