#11317: jQuery.ifNumeric #684

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@gibson042
Member

gibson042 commented Feb 14, 2012

jQuery Size - compared to last make
  250296   (+112) jquery.js
   94242    (+24) jquery.min.js
   33451     (-3) jquery.min.js.gz
@dmethvin

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Feb 14, 2012

Member

I'm not a fan of adding another utility-ish function that people can bikeshed. We're already getting that with isNumeric itself, like people who want floating-point numbers to be excluded.

The two changes in .css() can be done independently, so I think you are cheating on the byte count! :)

Other opinions?

Member

dmethvin commented Feb 14, 2012

I'm not a fan of adding another utility-ish function that people can bikeshed. We're already getting that with isNumeric itself, like people who want floating-point numbers to be excluded.

The two changes in .css() can be done independently, so I think you are cheating on the byte count! :)

Other opinions?

@@ -335,7 +334,7 @@ if ( !jQuery.support.opacity ) {
style.zoom = 1;
// if setting opacity to 1, and no other filters exist - attempt to remove filter attribute #6652
- if ( value >= 1 && jQuery.trim( filter.replace( ralpha, "" ) ) === "" ) {
+ if ( value >= 1 && !jQuery.trim( filter.replace( ralpha, "" ) ) ) {

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Feb 14, 2012

Member

Did you read the complete discussion here: http://bugs.jquery.com/ticket/6652 or the original patch #416? The check for an empty string is very much intentional. PLEASE read tickets that are cited before making changes like this.

cc @gnarf37

@rwaldron

rwaldron Feb 14, 2012

Member

Did you read the complete discussion here: http://bugs.jquery.com/ticket/6652 or the original patch #416? The check for an empty string is very much intentional. PLEASE read tickets that are cited before making changes like this.

cc @gnarf37

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Feb 14, 2012

Member

As a matter of fact, I did read http://bugs.jquery.com/ticket/6652. And I also read the source for jQuery.trim, which always returns a string. The equality check may be intentional, but it is not necessary.

@gibson042

gibson042 Feb 14, 2012

Member

As a matter of fact, I did read http://bugs.jquery.com/ticket/6652. And I also read the source for jQuery.trim, which always returns a string. The equality check may be intentional, but it is not necessary.

+ return jQuery.ifNumeric( value ) != null ?
+ ( current ? "" : " " ) + "alpha(opacity=" + value * 100 + ")" :
+ "";
+ });

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Feb 14, 2012

Member

How is this an improvement?

@rwaldron

rwaldron Feb 14, 2012

Member

How is this an improvement?

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Feb 14, 2012

Member

It is smaller (5 bytes gzipped) and saves a redundant regular expression test in the "replacement" case.

That being said, it may or may not be faster. I've opened a jsPerf, and would appreciate IE testing if someone can help.

@gibson042

gibson042 Feb 14, 2012

Member

It is smaller (5 bytes gzipped) and saves a redundant regular expression test in the "replacement" case.

That being said, it may or may not be faster. I've opened a jsPerf, and would appreciate IE testing if someone can help.

@rwaldron

This comment has been minimized.

Show comment Hide comment
@rwaldron

rwaldron Feb 14, 2012

Member

-1

There is no discussion, no documentation and it appears to increase the API surface area for very little benefit. The API ifNumeric itself if awkward, intuitively, I would want that to accept an expression to be evaluated and a callback, but I'd be way wrong.

Member

rwaldron commented Feb 14, 2012

-1

There is no discussion, no documentation and it appears to increase the API surface area for very little benefit. The API ifNumeric itself if awkward, intuitively, I would want that to accept an expression to be evaluated and a callback, but I'd be way wrong.

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Feb 14, 2012

Member

Regarding byte count: reverting the css.js work takes this change from -3 to +9. But I would exempt that from "cheating" because it is related code and would not have been analyzed otherwise. Also, I did say that the method came along with a reduction, not that it was solely responsible for one. ;)

Regarding the API: I'm not married to the name, although the best one is already taken.

Member

gibson042 commented Feb 14, 2012

Regarding byte count: reverting the css.js work takes this change from -3 to +9. But I would exempt that from "cheating" because it is related code and would not have been analyzed otherwise. Also, I did say that the method came along with a reduction, not that it was solely responsible for one. ;)

Regarding the API: I'm not married to the name, although the best one is already taken.

@timmywil

This comment has been minimized.

Show comment Hide comment
@timmywil

timmywil Feb 14, 2012

Member

This ticket has been closed: plugin.

Member

timmywil commented Feb 14, 2012

This ticket has been closed: plugin.

@timmywil timmywil closed this Feb 14, 2012

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