osrec / currencyFormatter.js

A super simple currency formatting library
MIT License
633 stars 61 forks source link

Best Practice for Customizing formatAll ? #24

Closed MikeSteigerwald closed 6 years ago

MikeSteigerwald commented 6 years ago

I hesitate to even call this an issue, since I'm just asking for advice. For our app, I expanded the matches[ i ] assignment in formatAll to make sure the existing string was a number (otherwise, I get errors) and to make sure it got applied to input elements (i.e., has value, not textContent)

` for (var i = 0; i < matches.length; ++i) {

        // handle, e.g., span, p, etc.
        if (!(isNaN(matches[i].textContent))) {
            var formattedString = formatter(matches[i].textContent);
            matches[i].textContent = formattedString;
        }

        // handle inputs
        if (!(isNaN(matches[i].value))) {
            var formattedString = formatter(matches[i].value);
            matches[i].value = formattedString;
        }
    }`

My two questions:

  1. Is there a better practice for avoiding the NaN problem.
  2. Is there a better practice for 'customizing' formatAll than hacking into currencyFormatter.js? I don't want to get caught off guard if I install an update.
osrec commented 6 years ago

Do you know which elements and fields contain numbers beforehand? If yes, you can just add a class to them ("currency", for example), and then let the formatAll function only format elements that match that class by passing in a selector. See example 4 in the docs :)

MikeSteigerwald commented 6 years ago

Yes, I do know which elements and fields contain numbers beforehand. Unfortunately, I occasionally miss something in validation which lets the user enter a non-numeric value. If that makes it all the way into formatAll, I get NaN results which I then confuse my users with. I've added the isNan checking to 'abort' the formatting if the actual input is not numeric.

The other change I've made is to apply the formatting to input elements, i.e., if the element has a value property, format it just like we do for the textContent property.

osrec commented 6 years ago

Apologies for the delay in replying. I think we might just add this into the library itself. I.e. if the value is non-numeric, replace it with a user-defined value. We'll look to release an update in the next few weeks, so look out for it in there :)

MikeSteigerwald commented 6 years ago

Superb! My gratitude for this package precludes any need to apologize for delays!

osrec commented 6 years ago

I believe the functionality you require has been added in the latest version (2.1.0):

OSREC.CurrencyFormatter.format('10', { currency: 'USD', valueOnError: 'Woops!' }); // Returns $10.00

OSREC.CurrencyFormatter.format('abcd', { currency: 'USD', valueOnError: 'Woops!' }); // Returns Woops!