Feedback on cmd/screenshot and the Golang client API
Closed this issue · 2 comments
VojtechVitek commented
https://github.com/pressly/screenshot/blob/master/cmd/screenshot/main.go#L15-L39
- Always check the error, never ignore it (Golang is very explicit about errors)
- Replace
var window [2]int
https://github.com/pressly/screenshot/blob/master/cmd/screenshot/main.go#L16-L18 withWindowWidth int
andWindowHeight int
on theScreenshotOptions{}
struct - Can we rename
ScreenshotOptions
toPage
orOptions
? -
PdfOptions
- I'm fine with a single argument toPDF()
as long as thePdfOptions
embed the aboveScreenshotOptions{}
- well need to use
io.ReadCloser
, so the consumer can close the Body
marcellerusu commented
The reason I changed page to screenshotOptions is because the only thing screenshot and pdf shares is the viewport which is the Window here, if we start sharing other things it just starts getting complex IMO. I'll add some more in depth usage by EOD
VojtechVitek commented
@MarcelRusu sounds good then. I thought PDF options shares much more stuff with screenshot options.