Skip to content

Added new captureOnCallback option#66

Open
jessegavin wants to merge 3 commits intovbauer:masterfrom
jessegavin:master
Open

Added new captureOnCallback option#66
jessegavin wants to merge 3 commits intovbauer:masterfrom
jessegavin:master

Conversation

@jessegavin
Copy link

@jessegavin jessegavin commented Oct 4, 2016 •
edited
Loading

This pull request implements a solution to issue #11 .

I am not sure whether captureOnCallback is the best name, but callback was already taken.

If captureOnCallback is true, we use the onCallback callback to initiate screen capture (rather than checking if the document is ready).

I tested this feature using both phantom and slimer engines and it seems to work great.

I used the following url as the test page:
https://presentation-ngwjsmdjlf.now.sh

<html>
  <head></head>
  <body>
    <h1 id="heading">My Content!</h1>
    <script>
      var h1 = document.getElementById('heading');


      // When the page load event fires, update text and make it red

      window.addEventListener('load', function() {
        h1.innerText = 'onLoad!';
        h1.style.color = 'red';
      });

      function duration() {
        var d = null;
        var parts = location.search.split('?t=');
        if (parts.length > 0) {
          d = parseInt(parts[parts.length-1], 10);
        }
        return isNaN(d) ? 0 : d;
      }


      // When this timeout fires, update text, make it green,
      // then tell PhantomJS we are ready!

      setTimeout(function() {
        h1.innerText = 'page is ready!';
        h1.style.color = 'green';
        if (typeof window.callPhantom === 'function') {
          window.callPhantom({ hello: 'world' });
        }
      }, duration());
      </script>
  </body>
</html>

I am happy to make changes if needed.

@jessegavin
Copy link
Author

jessegavin commented Oct 4, 2016

This PR could definitely help folks with single page apps / dynamic UIs as well. (If they're willing to add window.callPhantom() in their code). I think issue #62 may have benefitted from a feature like this.

@jessegavin
Copy link
Author

jessegavin commented Oct 13, 2016

@vbauer Any thoughts on this Pull Request?

@vbauer
Copy link
Owner

vbauer commented Oct 13, 2016

@jessegavin Sorry, I'm on a business trip now. I would be able to take a look a little bit later..

@sdlaursen
Copy link

sdlaursen commented Nov 12, 2016

@vbauer , I really need this in my project, so if I can anyway throw in a vote for this, please consider this my vote:-)

@vbauer
Copy link
Owner

vbauer commented Dec 14, 2016

@jessegavin Could you please resolve conflicts (and I will accept it)?

@jessegavin
Copy link
Author

jessegavin commented Dec 28, 2016

I just came back to this thread, I'll resolve conflicts now.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jessegavin @vbauer @sdlaursen