Skip to content

Add ability to control number of fractional digits - #62

Merged
sindresorhus merged 11 commits into
sindresorhus:mainfrom
nmoinvaz:features/precision
Feb 21, 2021
Merged

Add ability to control number of fractional digits#62
sindresorhus merged 11 commits into
sindresorhus:mainfrom
nmoinvaz:features/precision

Conversation

@nmoinvaz

@nmoinvaz nmoinvaz commented Aug 30, 2020

Copy link
Copy Markdown
Contributor

This should resolve #57.

@nmoinvaz
nmoinvaz marked this pull request as draft August 30, 2020 20:44
@nmoinvaz
nmoinvaz force-pushed the features/precision branch from 26c3b96 to 22b92c7 Compare August 30, 2020 21:19
@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
Copy Markdown
Contributor Author

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

Comment thread readme.md Outdated
Comment thread readme.md Outdated
Comment thread readme.md Outdated
Comment thread readme.md Outdated
Comment thread index.js Outdated
Comment thread readme.md Outdated
Comment thread index.d.ts Outdated
Comment thread index.d.ts Outdated
Comment thread index.d.ts
Comment thread test.js
@sindresorhus

Copy link
Copy Markdown
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

nmoinvaz commented Jan 2, 2021

Copy link
Copy Markdown
Contributor Author

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

@nmoinvaz

nmoinvaz commented Jan 2, 2021

Copy link
Copy Markdown
Contributor Author

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

nmoinvaz commented Jan 2, 2021

Copy link
Copy Markdown
Contributor Author

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
@nmoinvaz
nmoinvaz requested a review from sindresorhus January 2, 2021 04:37
@nmoinvaz
nmoinvaz force-pushed the features/precision branch from 1bbce52 to e19c385 Compare January 2, 2021 04:40
Comment thread index.d.ts Outdated
@sindresorhus

sindresorhus commented Jan 7, 2021

Copy link
Copy Markdown
Owner

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

nmoinvaz commented Jan 8, 2021

Copy link
Copy Markdown
Contributor Author

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
nmoinvaz force-pushed the features/precision branch from c2bd721 to 5640ac7 Compare January 8, 2021 02:05
@nmoinvaz

nmoinvaz commented Jan 8, 2021

Copy link
Copy Markdown
Contributor Author

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

@nmoinvaz
nmoinvaz requested a review from sindresorhus January 8, 2021 02:07
@nmoinvaz

nmoinvaz commented Jan 8, 2021

Copy link
Copy Markdown
Contributor Author

Requested changes have been made again.

Comment thread index.js
@@ -58,7 +58,7 @@ const toLocaleString = (number, locale, options) => {
let result = number;

Copy link
Copy Markdown
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 = {}.

@nmoinvaz nmoinvaz Jan 22, 2021

Copy link
Copy Markdown
Contributor Author

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?

Comment thread index.js Outdated
Comment thread readme.md Outdated
Comment thread index.d.ts
Comment thread index.d.ts
Comment thread index.js

if (number < 1) {
const numberString = toLocaleString(number, options.locale);
const numberString = toLocaleString(number, options.locale, localeOptions);

Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread test.js
@nmoinvaz
nmoinvaz requested a review from sindresorhus January 22, 2021 04:03
@nmoinvaz

Copy link
Copy Markdown
Contributor Author

Some of the requested changes have been made.

Base automatically changed from master to main January 24, 2021 06:04
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