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

Fix table headers #1003

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

Conversation

stormsilver
Copy link
Contributor

Two things in this PR (I know, I know - I am bad and I feel bad)

  1. Since this change Fixes #169. Text::Box :valign option. #788 my table headers with valign: :bottom are too close to the bottom border (the bottoms of my g's and p's and y's and q's are touching that border. Mama always said to mind your p's and q's - I guess this is what she was talking about. She's a lot smarter than I ever gave her credit for when I was 16). It looks like originally final_gap was included in the fix but then removed because... something? Adding it back makes things look good again, but I don't know what other problems this could cause.
  2. Since this change Don't mutate options hash in Document#start_new_page #963 my document margins aren't respected anymore. It turns out that the PR (accidentally?) changed from checking symbol options to checking string options. A single colon fixes the problem.

@pointlessone
Copy link
Member

@stormsilver Thank you for your contribution.

Could you please provide specs for the fixes so that these bugs would never come back?

@stormsilver
Copy link
Contributor Author

@pointlessone I added a spec for the margins fix, but I was really really really unsure how to test the vertical alignment. I poked around the tests from the original PR but they passed with or without my code change, which seemed... wrong.

This fixes a problem where table headers with valign: bottom
were too close to the bottom border.
@thbar
Copy link

thbar commented Jul 30, 2020

As pointed in #1137 (comment), I believe this PR is likely stale and has already been integrated (2.2.0...2.2.1) via a separate commit if I'm not mistaken!

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.

3 participants