Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to control number of fractional digits #62

Merged
merged 11 commits into from
Feb 21, 2021

Conversation

nmoinvaz
Copy link
Contributor

@nmoinvaz nmoinvaz commented Aug 30, 2020

This should resolve #57.

@nmoinvaz nmoinvaz marked this pull request as draft August 30, 2020 20:44
@nmoinvaz nmoinvaz changed the title Added precision/digits option. Added digits and fixed options. Aug 30, 2020
@nmoinvaz nmoinvaz marked this pull request as ready for review August 30, 2020 21:21
@nmoinvaz
Copy link
Contributor Author

I can remove fixed option if you don't want it.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
test.js Show resolved Hide resolved
@sindresorhus
Copy link
Owner

What's the use-case for the fixed option?

@sindresorhus sindresorhus changed the title Added digits and fixed options. Add digits and fixed options Dec 23, 2020
@nmoinvaz
Copy link
Contributor Author

nmoinvaz commented Jan 2, 2021

There was no use case for fixed, I just added it as an alternative to precision for completeness. I will remove it.

@nmoinvaz
Copy link
Contributor Author

nmoinvaz commented Jan 2, 2021

It appears the proper way to do this is not through toFixed or toPrecision but through locale options when calling number.toLocaleString(locale, options). I can expose a localeOptions option which would allow users to set it through localeOptions: {minimumFractionDigits: 2, maximumFractionDigits: 2}, or I can keep with the digits options digits: 2 with a note that using digits will automatically format the number using the system default locale.

@nmoinvaz nmoinvaz force-pushed the features/precision branch 5 times, most recently from 8d64572 to 9ddc532 Compare January 2, 2021 04:28
@nmoinvaz
Copy link
Contributor Author

nmoinvaz commented Jan 2, 2021

I have tried both ways, in two commits. I think the last commit is the best that has a localeOptions which is more configurable because both minimumFractionDigits and maximumFractionDigits can be set if desired.

@nmoinvaz nmoinvaz changed the title Add digits and fixed options Add ability to control number of fractional digits displayed Jan 2, 2021
@nmoinvaz nmoinvaz force-pushed the features/precision branch 2 times, most recently from d75314f to 1bbce52 Compare January 2, 2021 04:36
index.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

sindresorhus commented Jan 7, 2021

I agree with minimumFractionDigits and maximumFractionDigits, but they should be top-level options, not nested in localeOptions. And you need to document their default values.

@nmoinvaz
Copy link
Contributor Author

nmoinvaz commented Jan 8, 2021

The default values will be undefined because the when minimumFractionDigits and maximumFractionDigits is not set it will default to toPrecision(3) which has entirely different behavior.

@nmoinvaz
Copy link
Contributor Author

nmoinvaz commented Jan 8, 2021

Actually if you meant default value in the Object.assign for options, then I have included it.

@nmoinvaz
Copy link
Contributor Author

nmoinvaz commented Jan 8, 2021

Requested changes have been made again.

@@ -58,7 +58,7 @@ const toLocaleString = (number, locale, options) => {
let result = number;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to just do options = {}.

Copy link
Contributor Author

@nmoinvaz nmoinvaz Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I would check Object.keys(options).length > 0 in the second if statement?

index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
index.d.ts Show resolved Hide resolved
if (number < 1) {
const numberString = toLocaleString(number, options.locale);
const numberString = toLocaleString(number, options.locale, localeOptions);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const numberString = toLocaleString(number, options.locale, localeOptions);
const numberString = toLocaleString(number, options.locale, {
...{
options.minimumFractionDigits,
options.maximumFractionDigits
}
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

localeOptions should be undefined if both minimumFractionDigits and maximumFractionDigits is not set - so that it does not get localized by toLocaleString.

test.js Show resolved Hide resolved
@nmoinvaz
Copy link
Contributor Author

Some of the requested changes have been made.

Base automatically changed from master to main January 24, 2021 06:04
@sindresorhus sindresorhus changed the title Add ability to control number of fractional digits displayed Add ability to control number of fractional digits Feb 21, 2021
@sindresorhus sindresorhus merged commit 8388e69 into sindresorhus:main Feb 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New option for precision
2 participants