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

Calculate cell width of multi-line cells correctly #91

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

Calculate cell width of multi-line cells correctly #91

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Aug 14, 2017

Ensures that the optimal cell width is calculated correctly when its content has more than one line.

This is similar to #50 but with specs.

Fixes #49.

Ensures that the optimal cell width is calculated correctly when
the column's content contains more than one line.

Fixes #49
@@ -135,7 +135,13 @@ def text_box(extra_options={})
#
def styled_width_of(text)
options = @text_options.reject { |k| k == :style }
with_font { @pdf.width_of(text, options) }
if text.empty?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this case get properly handled by width_of?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, width_of handles it correctly, but line 142 would subsequently call #max on an empty array and return nil; thus the conditional.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally would've preferred .max || 0 but it's not critical.

Is this case covered by a spec?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think .max || 0 is probably more readable. I'll change that and add a spec.

@pointlessone
Copy link
Member

I'll take closer look a bit later. Thank you for your contribution.

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.

2 participants