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

Calculate requiredHeight based on tallest cell #29

Merged
merged 1 commit into from Mar 22, 2017

Conversation

Projects
None yet
6 participants
@RickMeasham
Contributor

RickMeasham commented May 20, 2016

Previously the requiredHeight for the row was set to the height of the first cell found in the row. If another cell happened to want to push the row height taller, the row height was not correctly calculated and it appeared the row would fit on the current page. Once we get to rendering, the row doesn't fit on the current page and flips itself to the next page -- without the headHeight adjustment to the row position. The THEAD then prints itself over the top of that row.

The processing time here will take longer as we now need to determine the contentLogicalHeight for every cell rather than just the first, but there's no way around that.

Calculate requiredHeight based on tallest cell
Previously the `requiredHeight` for the row was set to the height of the first cell found in the row. If another cell happened to want to push the row height taller, the row height was not correctly calculated and it appeared the row would fit on the current page. Once we get to rendering, the row _doesn't_ fit on the current page and flips itself to the next page -- without the headHeight adjustment to the row position. The THEAD then prints itself over the top of that row.

The processing time here will take longer as we now need to determine the `contentLogicalHeight` for every cell rather than just the first, but there's no way around that.
@ashkulz

This comment has been minimized.

Member

ashkulz commented May 20, 2016

Will review it in detail tomorrow, but looks good otherwise. Fantastic insight and debugging to identify the root cause of the issue 👍

@ashkulz

This comment has been minimized.

Member

ashkulz commented May 20, 2016

Can you send the files to the email address listed in my profile?

@d4ncer

This comment has been minimized.

d4ncer commented May 30, 2016

+1. Would definitely like to see this go through.

@ashkulz

This comment has been minimized.

Member

ashkulz commented Jun 1, 2016

Unable to review in detail, would try to do it soon.

@viswa18

This comment has been minimized.

viswa18 commented Jun 22, 2016

@RickMeasham Good work. I think it has overlapping issue with header and footer.

@RickMeasham

This comment has been minimized.

Contributor

RickMeasham commented Jun 25, 2016

@viswa18 Thanks .. but I'm not sure if you're saying my solution has an issue or not. If it does, can you please post details, an example, and a way to reproduce?

@jschroe212

This comment has been minimized.

jschroe212 commented Aug 19, 2016

Nice fix @RickMeasham. I see this as an isolated fix to what everyone has been complaining about and not sure how it would introduce what @viswa18 mentioned. Seems to me like the bug that he's referring to would have to already be present, so this wouldn't be going backward. Would love to hear an explanation of what was mentioned about overlapping.

But +1 to getting this merged.

@ashkulz ashkulz merged commit 51be553 into wkhtmltopdf:wk_4.8.7 Mar 22, 2017

@ashkulz

This comment has been minimized.

Member

ashkulz commented Mar 22, 2017

I haven't really tested the corner case mentioned here, but better to release a partial fix than to not do anything for so long.

Thanks a lot for the contribution, and extremely sorry for the delay! This will be a part of the 0.12.5 release.

@lzawadzki

This comment has been minimized.

lzawadzki commented Jul 30, 2018

❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment