-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Implement getLineWidth function #3324
Conversation
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 the PR. In addition to the notes below, please add the function also to types/index.d.ts
.
src/jspdf.js
Outdated
* @name getLineWidth | ||
*/ | ||
var getLineWidth = (API.__private__.getLineWidth = API.getLineWidth = function() { | ||
return out(hpf(scale(lineWidth)) + " w"); |
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.
getLineWidth
shouldn't write anything to the document. Just return lineWidth
.
src/jspdf.js
Outdated
* | ||
* @function | ||
* @instance | ||
* @returns {number} lineHeightFactor |
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.
copy/paste error
test/specs/jspdf.unit.spec.js
Outdated
@@ -898,7 +898,7 @@ describe("Core: Unit Tests", () => { | |||
doc.__private__.setCustomOutputDestination(writeArray); | |||
doc.__private__.setLineWidth(595.28); | |||
|
|||
expect(writeArray).toEqual(["1687.41 w"]); | |||
expect(writeArray).toEqual(doc.__private__.getLineWidth()); |
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.
Won't work after changing what the function returns. Just add an assertion like expect(doc.getLineWidth).toEqual(595.28)
@HackbrettXXX Updated! Should be all set. On a related note, I'm not able to get Chrome to run the unit tests succesfully. I'm running Chrome 96.0.4664.55 (Mac OS 10.15.7). It seems that many of the tests fail, with output that looks like:
Any idea why that might be the case? The tests otherwise run fine on Firefox 94.0 (Mac OS 10.15). Also, should we put a note in the I'd be glad to open an issue and/or PR for any of these. Let me know! Thank you! |
I think the issue is probably that you're running the tests on a Mac. Some of the tests run a little different on different platforms.
Definitively a good idea!
The node version shouldn't matter much AFAIK. We could add an engines field with node >= 10. Pull requests for these things are welcome ;) |
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.
Seems like some of the tests still fail. I think you can probably just update the references. If you're struggling with Chrome on Mac and the tests give different results in Firefox I can update them.
@HackbrettXXX Ran the test on Windows and the failing tests are showing up. I was able to update the reference pdf for It doesn't seem to be as trivial as generating the reference pdfs from another environment, though I'm new to the project. Can you please confirm on your end that updating the reference pdfs does not result in passing tests? If it is a matter of environment set up, I'd like to configure mine appropriately. If it isn't, are you be able to point me in the right direction to update any other references to On a possibly related note, the current version, on master, of the three files mentioned above contain an error and do not render properly. Opening with Adobe Acrobat Reader DC (build 21.7.20099 and on Mac and Windows), when scrolling to the second page I receive the error message |
Thanks for the update. Unfortunately, I don't know exactly how to configure the environment. I just know that it works on some machines and on others not. What we might do to improve the situation is to create/update the references also on CI. Let me update the references this time. I can confirm that the PDFs are indeed broken in Adobe Reader. I wasn't aware of this, but we should look into that. I'm opening a new issue for that: #3334. |
Addresses the need to create a
getLineWidth
function mentioned in issue 3312.