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

fix: allow input field to have zero value #1647

Closed
wants to merge 3 commits into from

Conversation

flaminic
Copy link
Contributor

@flaminic flaminic commented Dec 4, 2024

Description

This fixes changing number fields with value 0 to having value ""

Checklist

  • API docs are generated
  • Tests were added
  • Storybook demos were added

All points above should be relevant for feature PRs. For bugfixes, some points might not be relevant. In that case, just check them anyway to signal the work is done.


Screenshots

supporting text

@flaminic flaminic requested a review from a team as a code owner December 4, 2024 11:08
@dhis2-bot
Copy link
Contributor

dhis2-bot commented Dec 4, 2024

🚀 Deployed on https://pr-1647--dhis2-ui.netlify.app

@dhis2-bot dhis2-bot temporarily deployed to netlify December 4, 2024 11:12 Inactive
@flaminic flaminic changed the title Allow input field to have zero value fix: allow input field to have zero value Dec 4, 2024
Comment on lines 69 to 75
value={
value === null ||
value === undefined ||
Number.isNaN(value)
? ''
: value
}
Copy link
Member

Choose a reason for hiding this comment

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

Could this just use a nullish coalescing operator? Are we also trying to handle NaN falsy values or just trying to make sure 0 can be passed?

Suggested change
value={
value === null ||
value === undefined ||
Number.isNaN(value)
? ''
: value
}
value={value ?? ''}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, i was not sure here. I doubt NaN is ever going to be passed as a value, but I thought maybe safer to keep the previous behaviour for everything that is not 0, -0 or other forms of 0? But happy to change it as well. Do you think ?? would be better?

Copy link

sonarqubecloud bot commented Dec 4, 2024

@dhis2-bot dhis2-bot temporarily deployed to netlify December 4, 2024 12:52 Inactive
@flaminic flaminic requested a review from amcgee December 4, 2024 13:08
@amcgee
Copy link
Member

amcgee commented Dec 9, 2024

I'm fine with either here @flaminic !

The one important thing I think is that if we're intending to have defined behavior for NaN values (the solution as written now) then we should make sure we have a unit test for that case.

Copy link
Member

@amcgee amcgee left a comment

Choose a reason for hiding this comment

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

🦖 🎉

@flaminic flaminic closed this Dec 11, 2024
@flaminic flaminic deleted the allow-input-field-to-have-zero-value branch December 11, 2024 09:21
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.

4 participants