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

resolves #1121 read padding from correct box when creating new page #1122

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

resolves #1121 read padding from correct box when creating new page #1122

wants to merge 2 commits into from

Conversation

mojavelinux
Copy link
Contributor

No description provided.

@mojavelinux
Copy link
Contributor Author

The failures seem to be due to new cop rules that showed up which are not related to this change.

@mojavelinux mojavelinux force-pushed the issue-1121-read-correct-padding-on-page-create branch from 6c93344 to db58b6b Compare August 25, 2020 07:10
@mojavelinux
Copy link
Contributor Author

I believe this solution is still correct. It fixes at least 2 different cases Prawn mangles the margins (namely when using bounding_box or column_box).

@mojavelinux mojavelinux force-pushed the issue-1121-read-correct-padding-on-page-create branch from db58b6b to 427a0f9 Compare August 25, 2020 07:26
@mojavelinux mojavelinux linked an issue Jul 21, 2021 that may be closed by this pull request
Padding is read from wrong box when creating new page #1121
Open
@tomprats
Copy link
Contributor

I'm also running into this issue. Anything I can do to help move this along?

@pointlessone anything you see?

@mojavelinux
Copy link
Contributor Author

I'm willing to help to. I'll will rebase to fix the merge conflict if there is interest.

@pointlessone
Copy link
Member

A test would be nice to have.

@tomprats
Copy link
Contributor

Here's a test based on @mojavelinux's example in the issue tomprats@e46a4ee

And here's a branch with @mojavelinux's fix that allows the test to pass (but I didn't cherrypick it so it doesn't attribute the work properly) https://github.com/tomprats/prawn/tree/fix-padding.

@pointlessone - Let me know if you'd like more tests!

@mojavelinux - I can create a new PR if you're busy, or if you just add a similar test case after your rebase, we might be good to go!

@mojavelinux mojavelinux force-pushed the issue-1121-read-correct-padding-on-page-create branch from 427a0f9 to aee05e0 Compare January 24, 2022 08:33
@mojavelinux
Copy link
Contributor Author

@tomprats Thanks for the test! I added it to the PR and rebased.

I tried to come up with another test scenario, but I couldn't remember what the situation was that led me to sending this PR. I know it had something to do with indent. Regardless, I can confirm that without the patch, the test does not pass. So I think it's proving the need for the change perfectly.

@mojavelinux mojavelinux force-pushed the issue-1121-read-correct-padding-on-page-create branch from aee05e0 to ad86699 Compare January 24, 2022 08:47
@mojavelinux
Copy link
Contributor Author

I did a bit more investigation and discovered the scenario where I ran into this problem. However, I can't come up with any other example that demonstrates the problem without involving other libraries such as prawn-table. The issue comes down to the order in which the indentation is applied and the bounding box is restored. The steps need to be in the other order.

@pointlessone
Copy link
Member

Thank you for your contributions. I will take a look soon.

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

@pointlessone pointlessone Awaiting requested review from pointlessone

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Padding is read from wrong box when creating new page
3 participants
@mojavelinux @tomprats @pointlessone