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.

Sign up for GitHub

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

Jump to bottom

Added drawLine method to PDFPage #171

Merged
merged 2 commits into from
Sep 25, 2019
Merged

Added drawLine method to PDFPage #171

merged 2 commits into from
Sep 25, 2019

Conversation

samezyane
Copy link

Hi @Hopding

I added the drawLine function to PDFPage.
I tested the functionality using the scratchpad.
I run all unit tests:

  • yarn test
    and also all integration tests:
  • yarn apps:node
  • yarn apps:web

Everything is fine.

Regards,

Saeed

Hopding
Hopding previously requested changes Sep 3, 2019
View reviewed changes
Copy link
Owner

@Hopding Hopding left a comment

Choose a reason for hiding this comment

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

Hello @samezyane! Thank you for your work on this. I think it will be a very handy feature that pdf-lib users will find very useful.

These changes look pretty good overall! I've just left a couple of review comments for you. Other than that, I would like to have an integration test that exercises the changes introduced here. Would you mind updating one of the tests (for each environment) to call the new drawLine method?

src/core/operators/PDFOperatorNames.ts Show resolved Hide resolved
src/api/operators.ts Show resolved Hide resolved
src/api/PDFPage.ts Show resolved Hide resolved
@Hopding
Copy link
Owner

Hopding commented Sep 25, 2019

@samezyane I'm going to change the target branch for this PR to AddDrawLineMethod. From there, I'll go ahead and implement the changes I requested before merging it into master. That way I can save you some time and ensure this is merged in time for the next release (hopefully in a day or two).

@Hopding Hopding changed the base branch from master to AddDrawLineMethod September 25, 2019 01:35
@Hopding Hopding dismissed their stale review September 25, 2019 01:36

Making requested changes in future PR before merging to master

@Hopding Hopding merged commit c1e805a into Hopding:AddDrawLineMethod Sep 25, 2019
Hopding added a commit that referenced this pull request Sep 25, 2019
@Hopding
Add PDFPage.drawLine() method (#201)
09ff093 
* Added drawLine method to PDFPage (#171)

* Cleanup

* Refactor PDFPage.drawline API

* Update doc comment
@Hopding Hopding mentioned this pull request Sep 25, 2019
Merged
@Hopding
Copy link
Owner

Hopding commented Sep 25, 2019

@samezyane Thank you for your work on this! The PDFPage.drawLine method will be available in the next release of pdf-lib.

Hopding added a commit that referenced this pull request Aug 30, 2021
@Hopding
Add PDFPage.drawLine() method (#201)
96d03cc 
* Added drawLine method to PDFPage (#171)

* Cleanup

* Refactor PDFPage.drawline API

* Update doc comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hopding Hopding Hopding left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samezyane @Hopding