-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
fix #2938: textWithLink -> link centered correctly #3026
Conversation
Closing this PR as it is breaking the annotation test cases. Edit: Fixed it based on the feedback. |
src/modules/annotations.js
Outdated
x += width * 0.5; | ||
this.link(x - width, y - height, width, height, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should do this conditionally depending on the options.align
and options.maxWidth
properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @HackbrettXXX I guess you are right, checked it based on align condition. But due to some reason build got interrupted and ci throws error. Can you do a re-run or help me out on that part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should fix it for "right" and "justify" as well. See https://github.com/MrRio/jsPDF/blob/master/src/jspdf.js#L3776
@HackbrettXXX Justify seems working fine, just updated with right. Not sure why build is failing though after i synced again with master. Can you just re-run it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for incorporating the changes.
- Please add a test case. The second PDF looks good and would work well as test case: https://github.com/MrRio/jsPDF/blob/master/CONTRIBUTING.md#pull-requests
- For 'justify', I think we need a test with maxWidth set.
- Please run prettier before committing
not sure about the maxWidth set. can you brief it a bit? |
@HackbrettXXX need your help on this test case, using the same pdf output but its still throwing error. Am i missing anything? |
I think this is an encoding issue when saving or committing the PDF files. Happens on some machines and I haven't figured out why. |
I mean a test case with something like this:
|
@HackbrettXXX any work around for this? or any suggestion on how to go for the test cases, in those situation? |
In this case I can create the reference file and check it in. I will do so after you added a test case for "justify" with maxWidth. |
passing maxWidth not working on any alignment though. Is it a bug already? @HackbrettXXX |
Ah, ok, I just saw that the method was never designed to work with multiline strings. I guess then it would be fine if we don't support maxWidth for now. But implementing it should also be not difficult. We just need to consider the actual (multi-line) height of the text. We can get the height with |
Updated the changes as per request from here
#2964
Let me know if need any more improvement.