pressly/screenshot-nodejs

Feedback on cmd/screenshot and the Golang client API

Closed this issue · 2 comments

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 with WindowWidth int and WindowHeight int on the ScreenshotOptions{} struct
  • Can we rename ScreenshotOptions to Page or Options?
  • PdfOptions - I'm fine with a single argument to PDF() as long as the PdfOptions embed the above ScreenshotOptions{}
  • well need to use io.ReadCloser, so the consumer can close the Body

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

@MarcelRusu sounds good then. I thought PDF options shares much more stuff with screenshot options.