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 binding for Page.pdf() #26

Merged
merged 7 commits into from Mar 11, 2018

Conversation

Projects
None yet
2 participants
@jihchi
Copy link
Collaborator

jihchi commented Mar 10, 2018

Fix #4

Archie Lee added some commits Mar 10, 2018

Archie Lee
Archie Lee
Archie Lee
@@ -196,13 +196,13 @@ describe("Page", () => {
|> Browser.newPage
|> then_(page => {
let options = Navigation.makeOptions(~timeout=25000., ());
page |> Page.goto("https://google.com", ~options, ());
page |> Page.goto("file://" ++ testPagePath, ~options, ());

This comment has been minimized.

@jihchi

jihchi Mar 11, 2018

Author Collaborator

Before

Page
    ✓ goto() (325ms)

After

Page
    ✓ goto() (68ms)

This comment has been minimized.

@zploskey

zploskey Mar 11, 2018

Owner

Nice, I had been thinking of doing something like this as well. This has the added bonus that all the tests work offline.

@jihchi jihchi requested a review from zploskey Mar 11, 2018

@@ -196,13 +196,13 @@ describe("Page", () => {
|> Browser.newPage
|> then_(page => {
let options = Navigation.makeOptions(~timeout=25000., ());
page |> Page.goto("https://google.com", ~options, ());
page |> Page.goto("file://" ++ testPagePath, ~options, ());

This comment has been minimized.

@zploskey

zploskey Mar 11, 2018

Owner

Nice, I had been thinking of doing something like this as well. This has the added bonus that all the tests work offline.

src/Page.re Outdated
@@ -52,6 +52,30 @@ type emulateOption = {
"userAgent": string,
};

type boxModel = {

This comment has been minimized.

@zploskey

zploskey Mar 11, 2018

Owner

What do you think about just calling this margin? Or maybe marginOptions?

This comment has been minimized.

@jihchi

jihchi Mar 11, 2018

Author Collaborator

Fix in comment cb78ea6

src/Page.re Outdated
@@ -70,6 +94,50 @@ external makeCookie :
_ =
"";

[@bs.obj]
external makeBoxModel :

This comment has been minimized.

@zploskey

zploskey Mar 11, 2018

Owner

makeMargin?

This comment has been minimized.

@jihchi

jihchi Mar 11, 2018

Author Collaborator

makeMarginOptions ?

This comment has been minimized.

@jihchi

jihchi Mar 11, 2018

Author Collaborator

I think just makeMargin and type margin would be better.

This comment has been minimized.

@zploskey

zploskey Mar 11, 2018

Owner

Agreed.

src/Page.re Outdated
@@ -141,3 +209,6 @@ external emulateMedia :
external emulateMediaDisable :
([@bs.as {json|null|json}] _) => Js.Promise.t(unit) =
"emulateMedia";

[@bs.send.pipe: t]
external pdf : pdfOptions => Js.Promise.t(Js_typed_array.ArrayBuffer.t) = "";

This comment has been minimized.

@zploskey

zploskey Mar 11, 2018

Owner

We eventually want to use Node.buffer but bs-platform ships hardly any functions for it so I think we're still better off treating it like a typed array as you've done here.

This comment has been minimized.

@jihchi

jihchi Mar 11, 2018

Author Collaborator

Agree. I'll add comment in the code.

src/Page.re Outdated
~margin: boxModel=?,
unit
) =>
_ =

This comment has been minimized.

@zploskey

zploskey Mar 11, 2018

Owner

Why not specify the type pdfOptions here?

Archie Lee added some commits Mar 11, 2018

@zploskey zploskey merged commit e03d3a7 into zploskey:master Mar 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jihchi jihchi deleted the jihchi:bindings_for_page_pdf branch Mar 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment