-
Notifications
You must be signed in to change notification settings - Fork 99
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
Simple use case triggers unnecessary page break #22
Comments
@hbrandl: This is a fairly severe bug, it seems. Because I'm not sure if there's a workaround, I feel like this is something we should address sooner rather than later. If you know the source of the problem and can easily resolve it in the next day or two (w. a test to prevent regressions), let's try to cut a new release w. a fix. Otherwise, I think we should consider releasing a 0.1.2 that reverts the work done in 0.1.1, effectively bringing us back to the same behavior of 0.1.0 until the issue is resolved. (I'd probably cut a reverted release off a branch based on the 0.1.0 tag, to prevent sending master far back in time). Let me know what you'd like to do. |
I have met this issue yesterday. Because I haven't time to dig into it. My workaround is don't use :position => :center in the table method, and make it full width as the bounding box. Hope this issue can be fixed. |
@ruanwz: The code we're working on together is where I extracted this case from. 😁 The problem is that using the full width of the bounding box isn't a suitable workaround for all but the most trivial cases. Once you set the width to the full width of the box, it eliminates the point of trying to center the table, because it's filling the entire space. There's also a deeper problem, in that you can't even use a nested bounding box to work around this problem. So if you do something like this, you'd still get the unnecessary page break: bounding_box [bounds.width/2 - 200, bounds.top], :width => 400 do
text "HELLO"
table [["A", "B", "C"], ["D", "E", "F"]], :width => bounds.width
end That's why I'm worried about the potential impact of this bug. It's going to hit a lot of ordinary use cases. |
@sandal I aggree, that this is a severe bug. I'll try to fix it within 48 hours. If I can't make it by then, please go forward with the proposed plan of reverting to 0.1.0. This bug is probably related to pull request #23. Seems like I'll have to think of a strategy on how to address refactoring for the future. I was hoping that the test suite covered at least nearly all ordinary cases. |
@hbrandl: Thanks. Don't feel pressured to rush a fix, I don't view reverting as a bad choice unless this is an easy fix. We will get some things wrong as we try to fix up prawn-table, and so having to press an "undo" button once in a while is just fine! We could think about building an acceptance test suite of some sort... just a bunch of common use cases and then make some basic assertions about what we expect to happen. This would be easily caught by that sort of test, even if its only assertion was "Expect to fit on one page". |
fixes #22 - unnecessary page break with centered tables
@hbrandl Do you want to attempt to follow the release checklist and cut 0.1.2 yourself? https://github.com/prawnpdf/prawn-table/wiki/Release-checklist -- You can always email me if there are questions or if we snag on permissions issues anywhere. |
@hbrandl Please let me know about whether you plan to cut the 0.1.2 release in the next day or so. If not, I'll take care of it over the weekend. |
@sandal I was about to do it today, when I was interrupted. The changelog should be fine, version is 0.1.2 and the tests pass (but do check again just to be sure). Please go ahead and release it. I'll cut 0.1.3. |
@hbrandl OK, I'll take care of this when I get up tomorrow morning. Thanks for the quick turnaround on a fix! |
The following code will result in a two page PDF on prawn-table 0.1.1:
On 0.1.0, it works as expected, rendering the following results;
The text was updated successfully, but these errors were encountered: