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

Implement getLineWidth function #3324

Merged
merged 3 commits into from Dec 10, 2021
Merged

Implement getLineWidth function #3324

merged 3 commits into from Dec 10, 2021

Conversation

richardtorres314
Copy link
Contributor

Addresses the need to create a getLineWidth function mentioned in issue 3312.

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 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");
Copy link
Collaborator

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

copy/paste error

@@ -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());
Copy link
Collaborator

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)

@richardtorres314
Copy link
Contributor Author

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

x     respects width and windowWidth options
      Chrome 96.0.4664.55 (Mac OS 10.15.7)
    Expected $[12] = '/Length 3386' to equal '/Length 3392'.
    Expected $[39] = '0. 804.7 Td' to equal '0. 807.08 Td'.
    Expected $[50] = '111.06 804.7 Td' to equal '111.06 807.08 Td'.

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 CONTRIBUTING.md file about making sure to update types if there are changes? Lastly, it seems like we're using Node v14. Should we add the engines field to the package.json file for clarity?

I'd be glad to open an issue and/or PR for any of these. Let me know! Thank you!

@HackbrettXXX
Copy link
Collaborator

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

x     respects width and windowWidth options
      Chrome 96.0.4664.55 (Mac OS 10.15.7)
    Expected $[12] = '/Length 3386' to equal '/Length 3392'.
    Expected $[39] = '0. 804.7 Td' to equal '0. 807.08 Td'.
    Expected $[50] = '111.06 804.7 Td' to equal '111.06 807.08 Td'.

Any idea why that might be the case? The tests otherwise run fine on Firefox 94.0 (Mac OS 10.15).

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.

Also, should we put a note in the CONTRIBUTING.md file about making sure to update types if there are changes?

Definitively a good idea!

Lastly, it seems like we're using Node v14. Should we add the engines field to the package.json file for clarity?

The node version shouldn't matter much AFAIK. We could add an engines field with node >= 10.

Pull requests for these things are welcome ;)

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.

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.

@richardtorres314
Copy link
Contributor Author

richardtorres314 commented Dec 8, 2021

@HackbrettXXX Ran the test on Windows and the failing tests are showing up. I was able to update the reference pdf for html-margin-page-break and that passed the test but updating the remaining three did not result in passing tests (html-font-faces, html-margin-page-break-text, and html-margin-page-break-slice).

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 lineWidth that may have been updated with this change?

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 An error exists on this page. Acrobat may not display the page correctly. Please contact the person who created the PDF document to correct the problem. This error seems to happen even when I try to re-render the PDFs. Any idea what might be the reason or is this something being worked on? Thank you!

@HackbrettXXX
Copy link
Collaborator

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.

@HackbrettXXX HackbrettXXX merged commit e2d687c into parallax:master Dec 10, 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.

None yet

2 participants