search

maxwell / screencap

A gem to screencap webpages in ruby. Uses Phantom.js under the hood.
MIT License
179 stars 63 forks source link

Fix problem with TLS encoding. #18

Closed nachogiljaldo closed 7 years ago

nachogiljaldo commented 9 years ago

This PR is supposed to fix https://github.com/maxwell/screencap/issues/16 as well as allowing using any parameter from phantomjs that might be needed in the future.

skorecky commented 9 years ago

I know the build check is failing, but could this possible get fixed and merged in? Would really help me out on a project. @nachogiljaldo any chance you could recheck the specs?

nachogiljaldo commented 9 years ago

Build passes now. I removed the dependency with phantomjs.rb which was used only in development and was actually breaking the tests (not in my personal laptop, but it was on Travis and my job's one). Problem was that since there are dependencies with phantomjs and phantomjs.rb it created problems (which was anyway not a good idea, because even if I had made the tests pass with the phantomjs.rb dependency, that wouldn't mean it would work on a real deployment).

Let me know whether that's fine or not. In case it is not, I created https://github.com/westoque/phantomjs.rb/pull/14 so we could refactor to use instance run method rather than static one and provide the options we need (but as I said already, it is not a good idea IMHO, since that would work only for phantomjs.rb but not with phantomjs which, if I understand correctly, the one used at runtime).

nachogiljaldo commented 9 years ago

@maxwell could you please give your feedback on this and merge it is all fine? Thx!

nachogiljaldo commented 7 years ago

@maxwell any feedback on this?

maxwell commented 7 years ago

looks good to me