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

INVALID feature/59-allow-uncentralized-barcode-and-qrcode #94

Closed
wants to merge 3 commits into from

Conversation

SuddenGunter
Copy link
Collaborator

@SuddenGunter SuddenGunter commented Oct 17, 2019

Description
allows uncentralized barcode/qrcode
main.go only for testing, will be remove when pr would be ready for review.
Result pdf for current main.go:
1571337654_2140_17102019_343x765
Same main.go on current dev:
1571337775_2142_17102019_288x658

I see 2 related issues:

  • why qr code so blurry?
  • why barcode ignores proportion (16x9 vs 4x3)?

Related Issue
resolve #59
resolve #60

Checklist

check with "x", if applied to your change

  • All methods associated with structs has func (s *struct) method() {} name style.
  • Wrote unit tests for new/changed features.
  • Updated docs/doc.go
  • Updated pkg/pdf/example_test.go
  • Updated README.md
  • New public methods has comments upside them explaining what it does
  • Executed go fmt github.com/johnfercher/maroto/... to format all files

@codecov-io
Copy link

codecov-io commented Oct 17, 2019

Codecov Report

Merging #94 into dev will increase coverage by 0.68%.
The diff coverage is 93.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev      #94      +/-   ##
==========================================
+ Coverage   95.69%   96.36%   +0.68%     
==========================================
  Files           8        8              
  Lines         463      466       +3     
==========================================
+ Hits          443      449       +6     
+ Misses         16       13       -3     
  Partials        4        4
Impacted Files Coverage Δ
internal/code.go 100% <100%> (ø) ⬆️
pkg/pdf/pdf.go 91.5% <50%> (+1.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b630a80...4adad23. Read the comment docs.

@SuddenGunter
Copy link
Collaborator Author

codecov/patch — 93.34% of diff hit (target 95.67%)
codecov/project — 96.34% (+0.68%) compared to 87d45ab

@johnfercher do you have any idea what I've made wrong here?

@johnfercher
Copy link
Owner

  • why qr code so blurry?
    Strange, I will check later.

  • why barcode ignores proportion (16x9 vs 4x3)?
    In the master code it works, maybe your modification breaks the feature.

  • do you have any idea what I've made wrong here?
    The code in the maroto.go Barcode doesn't have unit tests yet, we should add because your modification affect that code. You could follow de QrCode unit test example.

@SuddenGunter
Copy link
Collaborator Author

SuddenGunter commented Oct 17, 2019

why barcode ignores proportion (16x9 vs 4x3)?
In the master code it works, maybe your modification breaks the feature.

I've tested code with dev without my changes - still wasn't working
UPD: will fix tests later tomorrow

allow uncentered  barcode

add main.go for manual testing

gofmt

add tests
@johnfercher
Copy link
Owner

johnfercher commented Oct 18, 2019

Ouch, there is an inverted if lol, I'm so sorry.

maroto/pkg/props/prop.go

Lines 138 to 140 in 76210ee

if r.Proportion.Height > r.Proportion.Width*0.33 {
r.Proportion.Height = r.Proportion.Width * 0.33
}

I will create another issue for that, thank you very much.

@SuddenGunter
Copy link
Collaborator Author

@johnfercher I guess I'd broke codecov when rebased branch and squashed some commits.
Can it happen, what do you think?
https://codecov.io/gh/johnfercher/maroto/compare/b630a80a0ee08a62b5a3047fc809a0e0a29744c3...00476f03195c85c336f7b9c3a86a7ebca6b61eb0

Missing base report. Unable to compare commits because the base of the compare did not upload a coverage report.
I'll delete this PR I guess and then recreate a new one. Sorry.

@SuddenGunter SuddenGunter changed the title feature/59-allow-uncentralized-barcode-and-qrcode INVALID feature/59-allow-uncentralized-barcode-and-qrcode Oct 22, 2019
@SuddenGunter
Copy link
Collaborator Author

@johnfercher pls mark as INVALID

@johnfercher johnfercher added the invalid This doesn't seem right label Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants