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

Allowing empty string as thousand separator in money formatter. #3587

Merged
merged 1 commit into from
Apr 20, 2022

Conversation

harbulot
Copy link
Contributor

@harbulot harbulot commented Feb 9, 2022

This is a feature request to be able to have an empty string as the thousand separator (money formatter).

At the moment, using thousand: "" in formatterParams is effectively ignored, since it falls back to the default separator (,).

This patch makes it possible not to use a thousand separator at all (i.e. use an empty string).

@olifolkerd
Copy link
Owner

Thanks for the pr,

If you want the facility to disable the thousand separator, then could you please make the disabling value false, not an empty string, that would then be consistent with other behaviour.

Also you shouldn't to a null check to check if the property has not need defined. You should use a typeof check looking to see if it is undefined.

If you could make those changes I would he happy to consider this for merging

Cheers

Oli

@harbulot
Copy link
Contributor Author

harbulot commented Feb 9, 2022

Sure, here it is!

I guess I thought the advantage of "" was that it didn't need to be documented as a special case (since it would do exactly what's intended with that string parameter value), but I also understand your reasoning behind using false.

Thank you!

@olifolkerd
Copy link
Owner

Thanks for making those updates. This looks good. I will approve it for merge and include it in the 5.2 release which should be comming out in april

@olifolkerd olifolkerd added the PR Awaiting Merge The PR is awaiting merge in next release version label Mar 11, 2022
@olifolkerd olifolkerd merged commit c25198d into olifolkerd:master Apr 20, 2022
@olifolkerd
Copy link
Owner

@harbulot if you are on the tabulator discord server, drop me a private message and i will give you a contributor role so other can see you have contributed to Tabulator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR Awaiting Merge The PR is awaiting merge in next release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants