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 option to pass wkhtmltopdf options. Fix #176. #185

Merged
merged 1 commit into from
Jan 25, 2018

Conversation

ryneeverett
Copy link
Contributor

It seems that some time in the last couple years wkhtmltopdf's default
margins were changed from '10mm' to zero. As an alternative to #177,
this PR adds an option to pass in arbitrary wkhtmltopdf long arguments
and sets the default top and bottom margin to '10mm'.

It seems that some time in the last couple years wkhtmltopdf's default
margins were changed from '10mm' to zero. As an alternative to hacksalot#177,
this PR adds an option to pass in arbitrary wkhtmltopdf long arguments
and sets the default top and bottom margin to '10mm'.
ryneeverett added a commit to ryneeverett/HackMyResume that referenced this pull request Jan 16, 2017
Note: This is based off
<hacksalot#185> because I changed
the generator expected arguments in that branch.
@peternowee
Copy link
Contributor

I tested this PR on the current dev branch. Just one remark:

  • The options file is JSON and the // comments shown in the example options file in README.md are actually not allowed. They will lead to the options not being loaded and the margins staying at the defaults of 10mm. I just created Mark options file example JSON and remove comments #192 to remove the existing comments, but not the new comment introduced here.

Other than that it looks good to me.

@ryneeverett
Copy link
Contributor Author

@peternowee I'm not strongly opinionated on #192 but I'd be happy to remove the comment if your patch is accepted. In the trade-off between copy-pastability and explanatory text I see no clear winner.

@peternowee
Copy link
Contributor

Yeah, it is just that it cost me some time to figure out. A warning by the software when failing to parse the options file could also do the trick.

@hacksalot hacksalot merged commit 7e2a3c3 into hacksalot:master Jan 25, 2018
@peternowee
Copy link
Contributor

FYI: This commit introduced another comment in the example JSON file (which JSON does not allow) in README.md, currently at line 494.

@hacksalot
Copy link
Owner

hacksalot commented Jan 26, 2018

Thanks Peter we'll touch this up prior to release.

To be clear, JSON doesn't allow comments but it's pretty common to see JSON annotated with comments in documentation. Here on GitHub, if I want annotated JSON in a README with syntax highlighting, I'll use js instead of json since JSON is legal JS and JS allows comments even if JSON doesn't:

// This is JSON that I want to add comments to
{
  "foo": "bar" // Blah blah blah
}

That's the kludge and that's the origin of the JSON + comments sprinkled throughout the READMEs. Of course, it makes it slightly more dangerous to copy-paste but my guess is, if you're using HackMyResume, with an options file of all things, you either already know comments aren't allowed in JSON, or can ferret it out when something breaks because of it.

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

3 participants