Skip to content

Conversation

injae-kim
Copy link

@injae-kim injae-kim commented Dec 30, 2023

PR Type

  • Bugfix

What is the current behavior?

Input (string) : "123456789.123456789"
Format : 0.0-9
Output :   123,456,789.12345679 // 7(8)9 -> 8 is lost..
Expected : 123,456,789.123456789

const num = strToNumber(value);
return formatNumber(num, locale, digitsInfo);

What is the new behavior?

        const formatStrs = [integerPart, fractionPart]
            .map(part => strToNumber(part))
            .map(num => formatNumber(num, locale, digitsInfo));
  • To format string to number on javascript well, apply formatting on integer/fraction part separately and then concat, return it.

Does this PR introduce a breaking change?

  • No

Copy link

google-cla bot commented Dec 30, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@injae-kim injae-kim force-pushed the 47809-fix-decimal-pipe-floating-point branch 3 times, most recently from 75c4a1c to b978193 Compare December 30, 2023 15:49
@injae-kim injae-kim force-pushed the 47809-fix-decimal-pipe-floating-point branch from b978193 to 9d5d49b Compare December 30, 2023 18:08
// e.g. '123456789.123456789'
if (typeof value === 'string' && value.includes('.')) {
const parts = value.split('.');
if (parts.length != 2) {
Copy link
Member

Choose a reason for hiding this comment

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

The condition doesn't cover every non valid string, we should likely rely on strToNumber to throw.

Copy link
Author

@injae-kim injae-kim Dec 30, 2023

Choose a reason for hiding this comment

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

      if (typeof value === 'string' && value.includes('.') && strToNumber(value)) {
        const parts = value.split('.');
        const integerPart = parts[0];
        const fractionPart = `0.${parts[1]}`;
        ..

Hmm you mean calling strToNumber(value) inside of if(..)?
Or can you give some example code? 🙇

Cause parts is array of string, we can't pass it to strToNumber(string) directly 🤔

Copy link
Member

@JeanMeche JeanMeche Dec 30, 2023

Choose a reason for hiding this comment

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

We can either call strToNumber() or probably better, extract the portion we'd like to reuse in a function :

function isStringNumber(value: number|string): boolean {
  return typeof value === 'string' && !isNaN(Number(value) - parseFloat(value))
}

Copy link
Author

Choose a reason for hiding this comment

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

aha~ I understood. isStringNumber looks much better! thanks for nice suggestion 👍

@injae-kim injae-kim force-pushed the 47809-fix-decimal-pipe-floating-point branch 2 times, most recently from edc5ec6 to 940e8d5 Compare December 30, 2023 18:54
fix(core): Fix decimal pipe floating point formatting bug
Address `JeanMeche`'s comments
Extract duplicated codes to isStringNumber()
@injae-kim injae-kim force-pushed the 47809-fix-decimal-pipe-floating-point branch from 940e8d5 to 92d36d0 Compare January 1, 2024 01:54
@AndrewKushnir AndrewKushnir requested a review from JoostK January 3, 2024 22:06
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Jan 3, 2024
@ngbot ngbot bot modified the milestone: Backlog Jan 3, 2024
@injae-kim
Copy link
Author

I need this feature and ready to update PR, so waiting your awesome reviews~ thanks! 😃

@injae-kim injae-kim requested a review from JeanMeche March 4, 2024 14:59
@pkozlowski-opensource pkozlowski-opensource removed the area: core Issues related to the framework runtime label Oct 3, 2024
@pkozlowski-opensource pkozlowski-opensource added the area: common Issues related to APIs in the @angular/common package label Oct 3, 2024
@pullapprove pullapprove bot removed the request for review from atscott October 3, 2024 17:42
@JeanMeche
Copy link
Member

Per #47809

After discussing it more recently with the team and given little to not interest shown from the community, we decided to not diverge from the native javascript behavior and keep the Decimal Pipe as is.

@JeanMeche JeanMeche closed this Mar 5, 2025
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants