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

fix #2938: textWithLink -> link centered correctly #3026

Merged
merged 9 commits into from
Dec 22, 2020
Merged

fix #2938: textWithLink -> link centered correctly #3026

merged 9 commits into from
Dec 22, 2020

Conversation

vbinithyanandamv
Copy link
Contributor

@vbinithyanandamv vbinithyanandamv commented Dec 9, 2020

Updated the changes as per request from here

#2964

Let me know if need any more improvement.

@vbinithyanandamv
Copy link
Contributor Author

vbinithyanandamv commented Dec 9, 2020

Closing this PR as it is breaking the annotation test cases.

Edit: Fixed it based on the feedback.

Comment on lines 366 to 367
x += width * 0.5;
this.link(x - width, y - height, width, height, options);
Copy link
Collaborator

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.

Copy link
Contributor Author

@vbinithyanandamv vbinithyanandamv Dec 10, 2020

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.

image

@vbinithyanandamv vbinithyanandamv changed the title fix #2938: textWithLink -> link centered correctly fix #2938: textWithLink --> link centered correctly Dec 10, 2020
@vbinithyanandamv vbinithyanandamv changed the title fix #2938: textWithLink --> link centered correctly fix #2938: textWithLink -> link centered correctly Dec 10, 2020
@vbinithyanandamv vbinithyanandamv changed the base branch from master to close-stale-issues December 10, 2020 12:43
@vbinithyanandamv vbinithyanandamv changed the base branch from close-stale-issues to master December 10, 2020 12:43
@vbinithyanandamv
Copy link
Contributor Author

center link.pdf

Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a 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

@vbinithyanandamv
Copy link
Contributor Author

vbinithyanandamv commented Dec 11, 2020

@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.

@vbinithyanandamv
Copy link
Contributor Author

textlink align (2).pdf

Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a 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.

@vbinithyanandamv
Copy link
Contributor Author

Thanks for incorporating the changes.

not sure about the maxWidth set. can you brief it a bit?

@vbinithyanandamv
Copy link
Contributor Author

@HackbrettXXX need your help on this test case, using the same pdf output but its still throwing error. Am i missing anything?

image

@HackbrettXXX
Copy link
Collaborator

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.

@HackbrettXXX
Copy link
Collaborator

not sure about the maxWidth set. can you brief it a bit?

I mean a test case with something like this:

doc.textWithLink('Justify aligned Link', (doc.internal.pageSize.getWidth() / 2), 30, { align: 'justify', url: 'https://www.google.com', maxWidth: 10 });

@vbinithyanandamv
Copy link
Contributor Author

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.

@HackbrettXXX any work around for this? or any suggestion on how to go for the test cases, in those situation?

@HackbrettXXX
Copy link
Collaborator

@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.

@vbinithyanandamv
Copy link
Contributor Author

vbinithyanandamv commented Dec 20, 2020

@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

@HackbrettXXX
Copy link
Collaborator

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 getTextDimensions. Your call ;)

This was referenced Mar 12, 2021
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