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

Issue #299 : Handle decimal values starting with ' . ' #340

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

ng185115
Copy link
Contributor

@ng185115 ng185115 commented Feb 13, 2024

Issue #299

  • This fix is specifically targeted to handle decimal input when ' . ' is entered.
  • Added neccessary tests.
  • Tests are passing locally.

Built and tested.

@ng185115
Copy link
Contributor Author

ng185115 commented Feb 13, 2024

Hi @cchanxzy

Can you please review this PR and if possible, could you consider merging this PR at your earliest convenience?

In this PR I have fixed it only for decimal numbers , if you want this to be applicable for other decimal separators as well I can modify this condition to

if (decimalSeparator && decimalSeparator !== '-' && value.startsWith(decimalSeparator)) { value = '0' + value; }

I'm excluding this ' - ' decimal separator as we doesn't know if that first character is intended to be the separator or negative sign.

@ng185115 ng185115 changed the title Issue #299 : Handle decimal values starting with . Issue #299 : Handle decimal values starting with ' . ' Feb 13, 2024
@cchanxzy
Copy link
Owner

Thanks for your PR @ng185115!

In this PR I have fixed it only for decimal numbers , if you want this to be applicable for other decimal separators as well I can modify this condition to

if (decimalSeparator && decimalSeparator !== '-' && value.startsWith(decimalSeparator)) { value = '0' + value; }

I'm excluding this ' - ' decimal separator as we doesn't know if that first character is intended to be the separator or negative sign.

Yes I think that's a good suggestion, if you update the PR with your suggestion I'll be happy to merge it.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (026329f) 100.00% compared to head (9907e29) 100.00%.
Report is 1 commits behind head on main.

❗ Current head 9907e29 differs from pull request most recent head 3c10ed1. Consider uploading reports for the commit 3c10ed1 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #340   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          411       413    +2     
  Branches       154       160    +6     
=========================================
+ Hits           411       413    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ng185115
Copy link
Contributor Author

Hi @cchanxzy
Made changes to support all decimal separators , Can you please review and merge it as soon as possible?

@cchanxzy cchanxzy merged commit 3b91498 into cchanxzy:main Feb 19, 2024
2 checks passed
@cchanxzy
Copy link
Owner

Merged. Thanks @ng185115 !

@ng185115 ng185115 deleted the pr/issue-299 branch February 19, 2024 15:30
@ng185115
Copy link
Contributor Author

Thanks @cchanxzy !!
When can we expect this to be published ?

Copy link

🎉 This PR is included in version 3.7.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@cchanxzy
Copy link
Owner

Ah just realised that the commit message didn't follow the semantic release convention, so it didn't trigger a release. That's on me, I should have checked.

I've just made a commit to trigger the release, so this commit will be included.

@ng185115
Copy link
Contributor Author

Thanks @cchanxzy !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants