Skip to content

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?

@pointlessone pointlessone force-pushed the master branch 2 times, most recently from 3c5df3e to 8309626 Compare January 22, 2017 21:18
@stormsilver stormsilver force-pushed the fix_table_headers branch 5 times, most recently from d54f1ce to 23ae60c Compare February 16, 2017 17:52
@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