Event: Added rcheckable to click.trigger function #3494
Conversation
|
|
|
@alexr101, thanks for your PR! By analyzing the history of the files in this pull request, we identified @timmywil, @gibson042 and @dmethvin to be potential reviewers. |
|
I just signed the CLA after finding the link in the details link for the check. It's still showing not signed...is there anything else I need to do? Thanks! |
src/event.js
Outdated
| "./var/rnothtmlwhite", | ||
| "./var/slice", | ||
| "./data/var/dataPriv", | ||
| "./manipulation/var/rcheckableType", |
There was a problem hiding this comment.
rcheckableType is no longer just for manipulation and should be promoted to a top-level var.
There was a problem hiding this comment.
@gibson042 thanks, got it. Now I'm new to the project so sorry if the question might seem silly...
You're basically asking to change to
"./var/rcheckableType",
And that would also mean I'd need to change the location of rcheckabletype in the file structure too since it's currently inside the manipulation folder and not on the top level var? Which would also require a change of the "./manipulation/var/rcheckableType" line on the other files that require it as well, correct?
thanks again
There was a problem hiding this comment.
Didn't know commits to my branch would automatically show up in the pull request. I thought they'd stay in my fork, and then I could send again to the pull request. I made some changes, but please ignore them. So I'll still need your input to confirm where I'm going with this. :)
gibson042
left a comment
There was a problem hiding this comment.
I made some changes, but please ignore them. So I'll still need your input to confirm where I'm going with this.
This is definitely on the right track, and thanks for updating. One more round should do it (and don't forget to run the unit tests!).
src/event.js
Outdated
| "./core/init", | ||
| "./selector" | ||
| ], function( jQuery, document, documentElement, rnothtmlwhite, slice, dataPriv ) { | ||
| ], function( jQuery, document, documentElement, rnothtmlwhite, slice, dataPriv, rcheckableType ) { |
There was a problem hiding this comment.
The parameter names need to line up with defined dependencies, so rcheckableType should precede slice.
| if ( rcheckableType.test( this.type ) && | ||
| this.click && jQuery.nodeName( this, "input" ) ) { | ||
| this.click(); | ||
|
|
There was a problem hiding this comment.
This blank is one line too low (it should follow the closing brace of the multi-line if condition, to separate it from its body).
ca7626a to
c0067fb
Compare
|
Ok, this should be up good now. I am sorry for the previous mess, I was worked on this from 3 different locations and each machine had certain issues. One I didn't have permissions on, so I couldn't install NPM, and all of them set default github usernames...which didn't let me pass the license/cla...but I fixed all of that by squashing the commits and changing the username and emails. Anyways...let me know if it's allright now! Thanks!!! |
| "./var/push", | ||
| "./var/rcheckableType", | ||
| "./core/access", | ||
| "./manipulation/var/rcheckableType", |
There was a problem hiding this comment.
If you're going to rearrange here, you also need to do so in the parameter list.
src/event.js
Outdated
| trigger: function() { | ||
| if ( this.type === "checkbox" && this.click && jQuery.nodeName( this, "input" ) ) { | ||
| if ( rcheckableType.test( this.type ) && | ||
| this.click && jQuery.nodeName( this, "input" ) ) { |
There was a problem hiding this comment.
There should be a blank line after the opening brace of the body here, to separate the multi-line condition from its block.
|
@gibson042 Got it, code should be up to standards now. Let me know if anything else needs changed. Also just a general question. On the Issues board are contributions generally only accepted for problems with the "help wanted" tab? There don't seem to be many of these. I want to keep familiarizing myself with jQuery's source code, and I would love to keep contributing on issues that are at my current level. |
|
Thanks @alexr101; I noticed that a test was missing so added it. As for issues, feel free to work on anything unassigned, but please post first to give a heads-up. Your contributions are appreciated! |
(cherry picked from commit b442aba) Ref jquerygh-3494 Fixes jquerygh-3423
Summary
Issue number #3423
Added radios to click.trigger function's conditional check. Originally it only filtered checkboxes, but now it filters all checkable types.
Checklist
Mark an
[x]for completed items, if you're not sure leave them unchecked and we can assist.Thanks! Bots and humans will be around shortly to check it out.