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

Add align type Justify to texts #460

Merged
merged 4 commits into from
Jun 25, 2024

Conversation

ArianeASA
Copy link
Contributor

Description

Added the justified alignment type for texts, which aligns the text with the margins.
The new type calculates the size of white space needed to completely fill a line.

image

Related Issue

Checklist

check with "x", ONLY IF APPLIED to your change

  • All methods associated with structs has func (<first letter of struct> *struct) method() {} name style.
  • Wrote unit tests for new/changed features.
  • Followed the unit test when,should naming pattern.
  • All mocks created with m := mocks.NewConstructor(t).
  • All mocks using m.EXPECT().MethodName() method to mock methods.
  • Updated docs/doc.go and docs/*
  • Updated example_test.go.
  • Updated README.md
  • New public methods/structs/interfaces has comments upside them explaining they responsibilities
  • Executed make dod with none issues pointed out by golangci-lint

Copy link

codecov bot commented Jun 23, 2024

Codecov Report

Attention: Patch coverage is 0% with 27 lines in your changes missing coverage. Please review.

Project coverage is 84.60%. Comparing base (8cc69ea) to head (cbcf3fe).

Files Patch % Lines
internal/providers/gofpdf/text.go 0.00% 27 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #460      +/-   ##
==========================================
- Coverage   85.71%   84.60%   -1.11%     
==========================================
  Files          61       61              
  Lines        2155     2174      +19     
==========================================
- Hits         1847     1839       -8     
- Misses        281      308      +27     
  Partials       27       27              
Flag Coverage Δ
unittests 84.60% <0.00%> (-1.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johnfercher
Copy link
Owner

Awesome contribution, I just added a couple of comments. Could you solve them?

docs/assets/examples/textgrid/v2/main.go Outdated Show resolved Hide resolved
Comment on lines 209 to 212
for _, word := range words {
s.pdf.Text(x, yColOffset+top, word)
x += s.pdf.GetStringWidth(word) + spaceWidth
}
Copy link
Owner

Choose a reason for hiding this comment

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

If you see the other alignments there are a logic as:

if textProp.Hyperlink != nil {
    s.pdf.LinkString(dx+xColOffset+left, yColOffset+top-fontHeight, textWidth, fontHeight, *textProp.Hyperlink)
    s.pdf.LinkString(dx+xColOffset+left, yColOffset+top-fontHeight, textWidth, fontHeight, *textProp.Hyperlink)
}

This is important to apply a link to text if required, I think that we have to add this here too. Otherwise, we will introduce a bug that links doesn't work with justify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip.
I added the hyperlink in the justified text.

@johnfercher
Copy link
Owner

johnfercher commented Jun 23, 2024

There are some lint issues pointed out too. About the unit tests you can let without. I'm about to refactor this text component entirely.

@ArianeASA ArianeASA requested a review from johnfercher June 23, 2024 21:53
@johnfercher johnfercher merged commit ecf13c1 into johnfercher:master Jun 25, 2024
3 of 5 checks passed
@johnfercher
Copy link
Owner

Awesome work @ArianeASA !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants