Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

rm redundant jQuery wrapping to find value of element #1512

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

No description provided.

Are you sure this change isn't against the style guide?

Yes. i checked. First commit is to make it conform to style guide.

@dmethvin dmethvin commented on the diff Mar 10, 2014

src/ajax/script.js
@@ -40,8 +40,7 @@ jQuery.ajaxTransport( "script", function( s ) {
async: true,
charset: s.scriptCharset,
src: s.url
- }).on(
- "load error",
+ }).on("load error",
@dmethvin

dmethvin Mar 10, 2014

Owner

Not sure why jscs didn't catch this, but it should be ( "

@dmethvin dmethvin and 1 other commented on an outdated diff Mar 10, 2014

src/attributes/val.js
@@ -71,14 +71,6 @@ jQuery.fn.extend({
jQuery.extend({
valHooks: {
- option: {
@dmethvin

dmethvin Mar 10, 2014

Owner

What was your logic for removing this? As I recall it's only needed in Blackberry (as the comment says) and IE6 (which it doesn't) but it's not related to this PR's topic AFAICT

@mgol

mgol Mar 16, 2014

Member

What was your logic for removing this? As I recall it's only needed in Blackberry (as the comment says) and IE6 (which it doesn't) but it's not related to this PR's topic AFAICT

Then we need to add a comment about that. :)

@dmethvin dmethvin commented on an outdated diff Mar 10, 2014

src/attributes/val.js
@@ -125,7 +117,7 @@ jQuery.extend({
while ( i-- ) {
option = options[ i ];
- if ( (option.selected = jQuery.inArray( jQuery(option).val(), values ) >= 0) ) {
+ if ( (option.selected = jQuery.inArray( option.value, values ) >= 0) ) {
@dmethvin

dmethvin Mar 10, 2014

Owner

Won't this break the case where the value attribute isn't specified and it's supposed to use the option text as a fallback? Do we have a unit test for that?

Owner

dmethvin commented Mar 14, 2014

@arjun024 Can you reply on the questions when you get a second? It may be fine but I'm counting on you to answer so I can be lazy. 😄

@dmethvin valhook for option was removed in a previous commit. (see fdd78fa)

@dmethvin option.value will also fallback to the the option text if value attribute isn't specified. the wrapping is redundant.

Owner

dmethvin commented Mar 18, 2014

What has me confused is that this has other patches in the diff other than yours. Can you rebase against the current master? Also, for your own sanity you should work in a branch and not in your master. If you are lucky it will be as easy as pull --rebase upstream master to rebase, but if you have made other changes since this it will be a mess.

@dmethvin I have rebased against the current master. Yes, I understand it's a bad idea to work on master. thanks for the tip :)

Owner

dmethvin commented Mar 21, 2014

Actually, option.value won't fall back reliably in all browsers so we have to leave that in. See

valHooks: {

@dmethvin dmethvin closed this Mar 21, 2014

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