m1l / Everything-Metric-Firefox

Firefox Addon for converting USCS sites to Metric / SI
https://addons.mozilla.org/en-US/firefox/addon/everything-metric-converter/
32 stars 7 forks source link

Use default locale string to format numbers #41

Closed qsantos closed 1 year ago

qsantos commented 1 year ago

Hello @m1l! I open this issue to continue the discussion following #40 regarding useComma and useSpaces.

The current implementation of formatNumber is:

function formatNumber(v, useCommaAsDecimalSeparator, useSpacesAsThousandSeparator) {
    if (useCommaAsDecimalSeparator) {
        const withThousandSeparator = v.toLocaleString('de-DE');
        if (useSpacesAsThousandSeparator) {
            return withThousandSeparator.replace(/\./g, '\u00A0');
        } else {
            return withThousandSeparator;
        }
    } else {
        const withThousandSeparator = v.toLocaleString('en-US');
        if (useSpacesAsThousandSeparator) {
            return withThousandSeparator.replace(/,/g, '\u00A0');
        } else {
            return withThousandSeparator;
        }
    }
}

Depending on useCommaAsDecimalSeparator, it uses the German or American locale to convert the number to a string. Then, when useSpacesAsThousandSeparator is true, it replaces the thousand separators by a space.

However, simply calling v.toLocaleString(), instead of v.toLocaleString('de-DE') or v.toLocaleString('en-US') is much faster, and would already format the number correctly according to the user's locale set in their browser. For instance, in a Firefox profile set in French:

image

My preferred resolution would be to simply switch to the default locale, and make away with useComma and useSpaces:

function formatNumber(v, useCommaAsDecimalSeparator, useSpacesAsThousandSeparator) {
    return v.toLocaleString();
}

An alternative to keep the old behavior for users who purposefully changed useComma and useSpaces would to add a switch useDefaultLocale. When the setting is first added, it would default to true, unless useComma and useSpaces have been changed by the user. Then, formatNumber would look like this:

function formatNumber(v, useDefaultLocale, useCommaAsDecimalSeparator, useSpacesAsThousandSeparator) {
    if (useDefaultLocale) {
        return v.toLocaleString();
    }
    if (useCommaAsDecimalSeparator) {
        const withThousandSeparator = v.toLocaleString('de-DE');
        if (useSpacesAsThousandSeparator) {
            return withThousandSeparator.replace(/\./g, '\u00A0');
        } else {
            return withThousandSeparator;
        }
    } else {
        const withThousandSeparator = v.toLocaleString('en-US');
        if (useSpacesAsThousandSeparator) {
            return withThousandSeparator.replace(/,/g, '\u00A0');
        } else {
            return withThousandSeparator;
        }
    }
}

What do you think?

m1l commented 1 year ago

OK, let's go for the toLocaleString only. This would simplify things

qsantos commented 1 year ago

Alright, I'll open a PR then! Of course, if it turns out that some users actually want the original behavior, we can always backtrack and restore the options.