Bug in extenders documentation #461

Closed
mikegleasonjr opened this Issue May 2, 2012 · 6 comments

3 participants

@mikegleasonjr

In the page:
http://knockoutjs.com/documentation/extenders.html

In the section "Live Example 1: Forcing input to be numeric"

If we leave an input blank or with only whitespaces, NaN is appearing in the input.

2 fixes in the extender are required:

  • in the "newValueAsNum" declaration and
  • in the condition near: "target.notifySubscribers(valueToWrite);"

Here's the full code fixed:

ko.extenders.numeric = function(target, precision) {
   //create a writeable computed observable to intercept writes to our observable
   var result = ko.computed({
      read: target,  //always return the original observables value
      write: function(newValue) {
         var current = target(),
            roundingMultiplier = Math.pow(10, precision),
            newValueAsNum = !/\S/.test(newValue) || isNaN(newValue) ? 0 : parseFloat(newValue),
            valueToWrite = Math.round(newValueAsNum * roundingMultiplier) / roundingMultiplier;

         //only write if it changed
         if (valueToWrite !== current) {
            target(valueToWrite);
         } else {
            //if the rounded value is the same, but a different value was written, force a notification for the current field
            if (newValue !== current) {
               target.notifySubscribers(valueToWrite);
            }
         }
      }
   });

   //initialize with current value to make sure it is rounded appropriately
   result(target());

   //return the new computed observable
   return result;
};

Thanks

@rniemeyer
Knockout.js member

Thanks for reporting this one. I just committed a fix for it. I did parseFloat(+newValue) to handle blank/whitespace and fixed the equality check on the last part as you had suggested.

@rniemeyer rniemeyer closed this May 2, 2012
@mikegleasonjr

Nice trick, the parseFloat(+newValue)

Thank you for this awesome library...

@stijnherreman

@rniemeyer +newValue gives you a number, while parseFloat takes a string, so when using this code in a TypeScript environment, it refuses to compile. I also found a question on SO questioning this change.

While it's just an example, it's up there on the official site, so I think the code should be correct.

@rniemeyer
Knockout.js member

@stijnherreman - I'll work on getting this updated

@stijnherreman

@rniemeyer great, thanks. Hope I didn't come off too strong there :)

@rniemeyer
Knockout.js member

@stijnherreman - no problem. The code was wrong. I believe it was originally just parseFloat and we needed to handle empty strings and strings with just spaces as well (isNaN(" ") is false) and I added the + without realizing that the parseFloat was then unnecessary.

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