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

Colspan fixes #620

Merged
merged 1 commit into from
Jan 2, 2014
Merged

Colspan fixes #620

merged 1 commit into from
Jan 2, 2014

Conversation

practicingruby
Copy link
Member

This is a continuation of work done on #579, as we revise it and prepare it to be merged. It addresses table colspan calculation related issues reported in #407.

@practicingruby
Copy link
Member Author

@hbrandl Note I've rebased your branch here into a single commit, but it's the same code.

@practicingruby
Copy link
Member Author

@hbrandl Sorry for the crazy rebasing and force-pushing. Check the files changed view now:
https://github.com/prawnpdf/prawn/pull/620/files

I basically only made two changes:

  • Added explicit comments (rather than expect to_not_raise) about CannotFitError
  • Extracted the column width calculations into their own class

The code in ColumnWidthCalculator is almost identical to what your original patch had, and it would benefit from being refactored further later. I'm also not necessarily committed to the names of anything in there. But I think it's a good idea for us to extract classes whenever a feature can mostly be handled in isolation, just to keep the existing classes from growing infinitely. We can contemplate proper design later.

Let me know if it's looking good to :shipit: now.

table_data = [["a", "b", "c"], [{:content=>"d", :colspan=>3}]]
column_widths = [50, 60, 400]

# Before we fixed #407, this line incorrectly raise a CannotFit error
Copy link
Member Author

Choose a reason for hiding this comment

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

@cheba, @bradediger Here's how I addressed the assertionless spec.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @sandal, this looks great.

practicingruby added a commit that referenced this pull request Jan 2, 2014
@practicingruby practicingruby merged commit 40e5be5 into master Jan 2, 2014
@practicingruby
Copy link
Member Author

Talked to @hbrandl over IRC. I'm merging this now, but anyone is welcome to open up another pull request to clean it up further.

@practicingruby practicingruby deleted the colspan-fixes branch January 2, 2014 18:36
TheSmartnik added a commit to Silverfin-Engineering/prawn that referenced this pull request Apr 15, 2022
TheSmartnik added a commit to Silverfin-Engineering/prawn that referenced this pull request Apr 15, 2022
TheSmartnik added a commit to Silverfin-Engineering/prawn that referenced this pull request Apr 15, 2022
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