search

codaxy / wkhtmltopdf

C# wrapper around excellent wkhtmltopdf console utility.
265 stars 92 forks source link

Support For Paper Types #28

Closed ChrisGarber-Medlever closed 7 years ago

ChrisGarber-Medlever commented 7 years ago

I added a static class which basically acts as a papertype enum for users. They can add a papertype to the output, which defaults to A4 if none. It checks against the full list of paper types for wkhtmltopdf to ensure that whatever a user passes is valid. If not it will default to A4.

ChrisGarber-Medlever commented 7 years ago

Oops, it looks like I only added this to the tests directory, sorry about that.

ChrisGarber-Medlever commented 7 years ago

and the more I look at it the more it looks like it doesn't really belong on the output class... I don't know I'll leave that up to you

mstijak commented 7 years ago

This is a very nice addition. Thanks.

There are a couple of small issues that I would change here. As you already noticed it would be better to move the option inside the PdfDocument class. Specifying paper size should be optional. Having that in mind, validation should be removed completely there is a list of standard paper sizes and wkhtmltpdf probably has its own validation. Also choosing a default paper size (A4) is not a good idea because that is a breaking change for existing users.

ChrisGarber-Medlever commented 7 years ago

But the A4 Size default is already built into the code (line 138), and into wkhtmltopdf. I guess we can rely on their default, but that will be a change to the existing code. I added the validation because a user could pass any string they wanted without selecting from the enum, but I suppose if I can apply the changes to the correct file and do another pull request?

mstijak commented 7 years ago

Ah, ok. I don't remember adding A4 as a default value, so I thought it's something new.

Regarding validation, I would either ditch it completely or throw an exception. This way it will confuse users.

If you make these changes and push them to your repository they will automatically added to the PR. GitHub is great :)

ChrisGarber-Medlever commented 7 years ago

Okay, went ahead and did that :-) I decided not to get rid of page-size completely and left in the A4 default because I'm not completely sure why it was there before, and perhaps removing it would be a breaking change.

mstijak commented 7 years ago

Perfect. Thanks.