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

Add Save as PDF option #1349

Merged
merged 3 commits into from
Dec 11, 2019
Merged

Add Save as PDF option #1349

merged 3 commits into from
Dec 11, 2019

Conversation

anthony-zhou
Copy link
Member

@anthony-zhou anthony-zhou commented Dec 10, 2019

Fixes #1284

I added a "Save as PDF" option for export, using the dependency jsPDF, which I installed in the project. Here is a screenshot of the options box:

image

Here is a screenshot of the PDF that was downloaded (I named the file "index.pdf" -- is that a good filename for now or is there something more descriptive we should change it to?):

image

Here is an example of a PDF generated with the new "Save as PDF" option:

image

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/is-reviewers for help, in a comment below
  • Insert-step functionality is working correct as expected.

@codecov
Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #1349 into main will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1349   +/-   ##
=======================================
  Coverage   66.57%   66.57%           
=======================================
  Files         125      125           
  Lines        2546     2546           
  Branches      397      397           
=======================================
  Hits         1695     1695           
  Misses        851      851

@anthony-zhou
Copy link
Member Author

@publiclab/is-reviewers How does this change look?

examples/demo.js Outdated
}

/*
* savePDF()
Copy link
Member

Choose a reason for hiding this comment

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

Could you please convert this to a JSDoc comment?

examples/demo.js Outdated
@@ -207,6 +209,39 @@ window.onload = function () {
});
}

//returns the data URL for the last image in the sequence
Copy link
Member

Choose a reason for hiding this comment

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

Please start with a capital letter, end with a . and add space between // and the sentence. We are trying to maintain consistency among all comments. Thank you.

harshkhandeparkar and others added 2 commits December 10, 2019 17:30
Edits follow the JSDoc guidelines. Comments begin with a capital letter and end with a period.
@anthony-zhou
Copy link
Member Author

@harshkhandeparkar Thanks for the feedback! I have edited the function comments to fit JSDoc comment guidelines, and the inline comments to start with a capital letter and end with a period. Is this the format you were looking for?

}

/**
* Download the given image URL as a PDF file.
Copy link
Member

Choose a reason for hiding this comment

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

Remove full stop from phrases.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it a complete sentence?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like the JSDoc examples usually look like this (they use imperative sentences).

@SidharthBansal
Copy link
Member

Do we actually need https://github.com/publiclab/image-sequencer/pull/1349/files#diff-32607347f8126e6534ebc7ebaec4853d in this pr?
Thanks both of you. I am not mentoring IS.
I think @keshav234156 can help you
Thanks

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a comment

Choose a reason for hiding this comment

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

LGTM

}

/**
* Download the given image URL as a PDF file.
Copy link
Member

Choose a reason for hiding this comment

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

I think you should use the @description directive instead of direct comment.

* @description Download.....

@jywarren
Copy link
Member

This looks great. Thank you so much!

@jywarren jywarren merged commit 5421a80 into publiclab:main Dec 11, 2019
jywarren pushed a commit that referenced this pull request Dec 16, 2019
* Add Save as PDF option

* Edit coments for consistency

Edits follow the JSDoc guidelines. Comments begin with a capital letter and end with a period.
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.

Add Save as PDF option to export the processed image
4 participants