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

Change movement depending on vertical table alignment #128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rsynnest
Copy link

@rsynnest rsynnest commented Sep 5, 2021

This is a potential fix for #127

It's a cheeky fix, but I think the end result is much better.

Before:
image

After:
image

Test PDF was generated using this code:

Prawn::Document.generate('result.pdf') do

data = [ ["TOP", "", ""], ]
table data, :cell_style => { :valign => :top}
move_down 20

data = [ ["CENTER", "", ""], ]
table data, :cell_style => { :valign => :center}
move_down 20

data = [ ["BOTTOM", "", ""], ]
table data, :cell_style => { :valign => :bottom}
move_down 20

end

@rsynnest
Copy link
Author

rsynnest commented Sep 5, 2021

Here is a PDF showing the results rendered for the 15 built-in Prawn fonts:
result.pdf

The only ones that have a problem are Symbol and ZapfDingbats, but those would also blow up with the pre-PR code, because the bottom-alignment code is unchanged.

@rsynnest
Copy link
Author

rsynnest commented Sep 6, 2021

Someone let me know that TTF fonts might have negative results after this change. I went ahead and generated a PDF using 310 TTF fonts available in Windows 10.

Before this PR:
before.pdf

After this PR:
after.pdf

Most TTF fonts appear to benefit from this PR, and at the worst, they remain the same broken result as before the PR (usually Top and Center are just too low). I given that valign seems to be broken currently for ALL fonts, including the builtin ones, this fix at least fixes built in fonts, and seems to also benefit most TTF fonts in testing.

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

Successfully merging this pull request may close these issues.

1 participant