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

Display a count of 0 when the prep count has not been defined instead of NaN #4006

Merged
merged 6 commits into from Sep 27, 2023

Conversation

CarolineDenis
Copy link
Contributor

Fixes #3881

@CarolineDenis CarolineDenis changed the base branch from production to v7.9-dev September 13, 2023 21:12
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

hmm, I didn't look into the code too closely, but NaN is not expected result - it implies that there might be some wrong calculation going on somewhere (either division by 0, or arithmetic on non-numeric data)

please double check where NaN is coming from and correct the issue there

i.e before the calculation that could result in a NaN, add a check to guard against such case, - if such case is detected, then set 0 instead of doing the calculation

tldr: the code change you made is correct, but it is better to prevent NaN from being created rather than changing how NaN is being displayed

small thing, but also prefer using Number.isNaN over isNaN (isNaN is legacy and has some weird behavior that has been fixed in Number.isNaN)

@CarolineDenis
Copy link
Contributor Author

hmm, I didn't look into the code too closely, but NaN is not expected result - it implies that there might be some wrong calculation going on somewhere (either division by 0, or arithmetic on non-numeric data)

please double check where NaN is coming from and correct the issue there

i.e before the calculation that could result in a NaN, add a check to guard against such case, - if such case is detected, then set 0 instead of doing the calculation

tldr: the code change you made is correct, but it is better to prevent NaN from being created rather than changing how NaN is being displayed

small thing, but also prefer using Number.isNaN over isNaN (isNaN is legacy and has some weird behavior that has been fixed in Number.isNaN)

ok will do that. The NaN comes from the fact that the count has not been defined (the field is just empty). If the user sets it to 0 we would see 0.

Triggered by b36dc73 on branch refs/heads/issue-3881
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

Better but not quite there. I probably wasn't clear, so let me clarify - NaN should never be created - yes, back-end returns null, but front-end converting that to NaN is not right.

The bug occurs here:

Screenshot 2023-09-14 at 18 23 24

See how line 194 uses Number.parseInt? that results in a NaN, instead, it should use f.parseInt(prepData[10] ?? undefined) ?? 0 like previous lines


for why I (and the JS community in general) are about trying to avoid NaNs as much as possible see any of the following:

@CarolineDenis CarolineDenis requested a review from a team September 20, 2023 13:25
@CarolineDenis CarolineDenis added this to the 7.9.1 milestone Sep 20, 2023
@grantfitzsimmons grantfitzsimmons changed the base branch from v7.9-dev to production September 26, 2023 02:41
@CarolineDenis CarolineDenis removed the request for review from melton-jason September 26, 2023 15:03
Copy link

@bronwyncombs bronwyncombs left a comment

Choose a reason for hiding this comment

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

working!

Screen.Recording.2023-09-27.at.3.24.20.PM.mov
Screen.Recording.2023-09-27.at.3.29.01.PM.mov

@CarolineDenis CarolineDenis merged commit 92c17aa into production Sep 27, 2023
9 checks passed
@CarolineDenis CarolineDenis deleted the issue-3881 branch September 27, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Loan/Gift preparations without count display as NaN on preparation selecting dialog
5 participants