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

InputNumber: onChange parameter type #5037

Closed
AryamnovEugeniy opened this issue Oct 13, 2023 · 4 comments
Closed

InputNumber: onChange parameter type #5037

AryamnovEugeniy opened this issue Oct 13, 2023 · 4 comments
Labels
Fixed patch Completed issues that will be published with next patch (1.0.X)

Comments

@AryamnovEugeniy
Copy link

What package has an issue

@mantine/core

Describe the bug

I noticed that in the latest version onChange in InputNumber is called with a string as its parameter when defaultValue is undefined. I am not sure this is correct, and it is definitely not correct for my situation.
Although of course there is a simple workaround. The problem for me is that I do not think that current behaviour is correct.

What version of @mantine/* packages do you have in package.json? (Note that all @mantine/* packages must have the same version in order to work correctly)

7.1.3

If possible, please include a link to a codesandbox with the reproduced problem

https://codesandbox.io/s/mantine-forked-xq74sl?file=/src/App.tsx

Do you know how to fix the issue

None

Are you willing to participate in fixing this issue and create a pull request with the fix

None

Possible fix

I found a PR that introduced this change: #4916
Possibly, when _value is undefined, we should pass a number to onChange. What do you think? Although I did not test this solution for these issues: #4901 , #4912
And to be honest, I am not sure that we should ever pass a string as a parameter to onChange (except maybe when there is no value, but in this case null is a better option from my point of view) - this is NumberInput after all =)

@7iomka
Copy link
Contributor

7iomka commented Oct 13, 2023

I agree with you that the type of string inside NumberInput make weird typings and mappings.
For example in my use-cases I need to refactor onChange prop on all NumberInput based components to something like
this:

  • v?: number
      onChange={(val) => onChange(val === '' ? undefined : (val as number))}
  • v: number | null
      onChange={(val) => onChange(val === '' ? null : (val as number))}
  • v: number
      onChange={(val) => onChange(val as number)}

The last one has issues of course.
So, I don't know what is the best way to handle empty NumberInput value with better types alongside with safety.

@odbayar
Copy link

odbayar commented Nov 3, 2023

Also the type of the value changes to number when you use the arrow buttons to increase and decrease the value, and it changes back to string when you use your keyboard to type numbers.

@7iomka
Copy link
Contributor

7iomka commented Nov 3, 2023

Also the type of the value changes to number when you use the arrow buttons to increase and decrease the value, and it changes back to string when you use your keyboard to type numbers.

Same issue.
I think it is related with wrong or inconsistent handling of value, because react-input-format give us string and float value, but mantine not.
telegram-cloud-photo-size-2-5334748595003446414-y

rtivital added a commit that referenced this issue Nov 12, 2023
@rtivital rtivital added the Fixed patch Completed issues that will be published with next patch (1.0.X) label Nov 12, 2023
@rtivital
Copy link
Member

Fixed in 7.2.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed patch Completed issues that will be published with next patch (1.0.X)
Projects
None yet
Development

No branches or pull requests

4 participants