iLoveElysia/pjax
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 9 Wiki Activity

Use the same options object in handle-response as in send-request #148

Merged
BehindTheMath merged 3 commits from fix/options-and-state into master 2018-04-26 08:27:51 -05:00
Conversation 13 Commits 3 Files Changed 5 +1
1 changed files with 1 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 1aebbcc502 - Show all commits

1
lib/proto/attach-form.js
View File

@@ -40,6 +40,7 @@ var formAction = function(el, event) {
var tagName = element.tagName.toLowerCase()
robinnorth commented 2018-04-13 03:55:49 -05:00 (Migrated from github.com)
Review
Copy Link

Did you meant to remove this?

Did you meant to remove this?
BehindTheMath commented 2018-04-13 08:39:45 -05:00 (Migrated from github.com)
Review
Copy Link

Yes. The tests run fine without it, so it's not necessary.

I should have added it to #147, but I forgot to do it before I merged it.

Yes. The tests run fine without it, so it's not necessary. I should have added it to #147, but I forgot to do it before I merged it.
robinnorth commented 2018-04-13 09:48:33 -05:00 (Migrated from github.com)
Review
Copy Link

If you're explicitly disabling a test for a given code block with an inline comment, I think you should always re-enable it after to ensure that you don't accidentally lose coverage... or, alternatively, just disable the rule globally if you don't find it to be of any use.

In this case, // jscs:disable disallowImplicitTypeConversion should be paired with a // jscs:enable disallowImplicitTypeConversion somewhere or we should disable the rule altogether, IMO. Personally, I'd be quite happy to disable it, as I find !!element.name to be quite a convenient shorthand for casting to a Boolean.

If you're explicitly disabling a test for a given code block with an inline comment, I think you should always re-enable it after to ensure that you don't accidentally lose coverage... or, alternatively, just disable the rule globally if you don't find it to be of any use. In this case, `// jscs:disable disallowImplicitTypeConversion` should be paired with a `// jscs:enable disallowImplicitTypeConversion` somewhere or we should disable the rule altogether, IMO. Personally, I'd be quite happy to disable it, as I find `!!element.name` to be quite a convenient shorthand for casting to a Boolean.
BehindTheMath commented 2018-04-22 17:23:09 -05:00 (Migrated from github.com)
Review
Copy Link

My bad. I just glanced quickly at it and I thought they were both disable rules, applicable to only the next line.

I'll revert that. Thanks for catching that.

My bad. I just glanced quickly at it and I thought they were both `disable` rules, applicable to only the next line. I'll revert that. Thanks for catching that.
// jscs:disable disallowImplicitTypeConversion
if (!!element.name && element.attributes !== undefined && tagName !== "button") {
// jscs:enable disallowImplicitTypeConversion
var type = element.attributes.type
if ((!type || type.value !== "checkbox" && type.value !== "radio") || element.checked) {