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

dev/core#343 Fix long address lines overflowing label #12691

Merged
merged 3 commits into from
Aug 28, 2018

Conversation

andrewpthompson
Copy link
Contributor

@andrewpthompson andrewpthompson commented Aug 20, 2018

Overview

When a name or address is very long (long enough for the text to wrap onto a new line) it overflows the label. This can be seen by comparing the PDF output by CiviCRM with an official Avery template. This fixes that by modifying CRM_Utils_PDF_Label::generateLabel() so that the width that it uses has the left & right padding subtracted from the label's width.

Before

Long lines can overflow the label boundary.

After

Long lines wrap within the label boundary with correct padding applied.

https://lab.civicrm.org/dev/core/issues/343

generateLabel() - Width did not take into account paddingLeft
@civibot
Copy link

civibot bot commented Aug 20, 2018

(Standard links)

@andrewpthompson andrewpthompson changed the title Fix long address lines overflowing label dev/core#343 Fix long address lines overflowing label Aug 20, 2018
@aydun
Copy link
Contributor

aydun commented Aug 21, 2018

Tested, works as described. Good to merge.

@@ -185,7 +185,7 @@ public function LabelSetFormat(&$format, $unit) {
*/
public function generateLabel($text) {
$args = array(
'w' => $this->width,
'w' => $this->width - 2 * $this->paddingLeft,
Copy link
Contributor

Choose a reason for hiding this comment

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

2 is a magic number here. Could you please add a comment explaining why this change works? Thanks!

@andrewpthompson
Copy link
Contributor Author

Thanks @MegaphoneJon and @aydun for the feedback. I've added a comment in response to Jon's review comment (good point).

@eileenmcnaughton
Copy link
Contributor

I feel review here is adequate - merging

@eileenmcnaughton eileenmcnaughton merged commit 5630f7a into civicrm:master Aug 28, 2018
@andrewpthompson andrewpthompson deleted the dev-core-343 branch August 29, 2018 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants