Updates to PR #21 - passPageOptions from Gruntfile.js to Phantom #47
Conversation
Merged in from STRML/grunt-lib-phantomjs@e356ada Much simpler method to pass page options into constructor.
|
Hey @bdowling. I came here from the jQuery UI PR and since I'm also a grunt-contributor, I started reviewing this. I have a hard time trawling through the previous PRs and their history. One thing I very much want to see here: Squash all the commits! Into one, maybe two, if there's a good reason. Then make sure the commit message explains this change. I will also comment on a few of the code changes. Sorry if this duplicates previous discussions... |
| @@ -50,13 +81,25 @@ module.exports = function(grunt) { | |||
| var options = this.options(); | |||
| var phantomjs = require('./lib/phantomjs').init(grunt); | |||
|
|
|||
| var server = null; | |||
| if (options.server) { server = require(options.server); } | |||
There was a problem hiding this comment.
What is this server option? There's a README update below, but it isn't mentioning this. The server variable also doesn't seem to have any purpose.
There was a problem hiding this comment.
So this seems solely for testing the page option, which should be fine. But the var server is not used anywhere, can that be removed?
Also, since you're using the server property a single time, is there a way to 'inline' it?
|
So the server component was established in the orig #21, but I can see how it is a bit confusing. I removed some extraneous code around that, so it is only loaded once for the header reflection test. You're not asking for the headers_server.js to be inlined into the Gruntfile, are you? I removed the comments in the test, but some I think were just copied from basic.html. Let me know if there is anything else and I can look at squashing as requested. |
Merged in from STRML/grunt-lib-phantomjs@e356ada (Essentially merged and squashed commits of gruntjs#21 and gruntjs#47 together)
|
Ok, so I haven't done a squashed merge before, does this branch look better? https:/bdowling/grunt-lib-phantomjs/tree/pr21-additions-squash And I don't think it is good practice to replace this branch because that'd change history. So if I can't fix this PR, the right thing to do just open up another PR? |
|
Replacing this request with #49 squashed pull. This one can be closed (for some reason I don't have that option). |
This is an update to PR #21 (not trying to steal any credit here!) just wanted to make this long running feature request easier to merge with latest master and combine the good ideas of others.
Credit to @pconerly and @jakerella with improvements from @ssafejava .
Hoping someone from @gruntjs team (@cowboy or @jsoverson, et al? ;) can have a moment to look at this and let me know if there is feedback or this can be merged. Others have mentioned it's useful, and I found it helped with some jquery-ui tests as well (jquery/jquery-ui#1144)