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

Solving Issue #775 #786

Closed
wants to merge 5 commits into from
Closed

Conversation

Mario1988
Copy link
Contributor

I improved the method add_fragment_to_line to get rid of the unexpected behavior:
If the last character of segment is a soft hyphen I subtract the width of one soft hyphen from the @accumulated_width.

In addition to that I add a Spec for issue #775 and I improved the spec for issue #347.

@practicingruby
Copy link
Member

@Mario1988: Can you look into the test suite failure? It may just be a broken mock object, but we need to sort that out before we can merge.

@practicingruby
Copy link
Member

I figured out what was causing the test failure: The width of soft hyphens was being computed whenever any fragment was run in any text call, even if the string had no soft hyphens in it. I moved that check into the if segment[-1] == soft_hyphen to solve this problem.

I've merged a squashed version of this patch in 766a534, thanks!

Some notes for future pull requests:

  • You have commit access to Prawn, so please build your feature branches off of our main repo rather than a fork. If you had done that, I could have added my commits directly to this pull request, making it easier to see what changes I made, and any other PrawnPDF contributor could have helped revise the pull request, too.
  • When you have a refactoring and a fix you want to do, open a separate patch for each. I was confused by the merge conflicts I saw on the changes to the tests for Hyphenation character rendered in last line of each paragraph of column_box #347, because it's unrelated to Problem with soft-hyphens and text-width #775.

These are minor issues, but keeping them in mind will help make my job easier. Thanks again for the fix!

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.

None yet

2 participants