fix: onValueChange not called for values with 3 or less characters
#842
Conversation
| @@ -191,17 +191,14 @@ export default function NumberFormatBase<BaseType = InputAttributes>( | |||
|
|
|||
| /** | |||
| * if the formatted value is not synced to parent, or if the formatted value is different from last synced value sync it | |||
| * we also don't need to sync to the parent if no formatting is applied | |||
There was a problem hiding this comment.
This behaviour contradicts what is described in the documentation:
This handler provides access to any values changes in the input field and is triggered only when a prop changes or the user input changes
value is a prop, so any change should trigger the onValueChange. If the original intent of the function was to trigger on formatting changes, it should have been better described and/or be a different prop.
| it('should not call onValueChange if no formatting is applied', async () => { | ||
| const mockOnValueChange = vi.fn(); | ||
|
|
||
| const { rerender } = await render(<NumericFormat value="" onValueChange={mockOnValueChange} />); | ||
|
|
||
| expect(mockOnValueChange).not.toHaveBeenCalled(); | ||
|
|
||
| rerender(<NumericFormat value={NaN} onValueChange={mockOnValueChange} />); | ||
| expect(mockOnValueChange).not.toHaveBeenCalled(); | ||
|
|
||
| rerender(<NumericFormat value={1234} onValueChange={mockOnValueChange} />); | ||
| expect(mockOnValueChange).not.toHaveBeenCalled(); | ||
|
|
||
| rerender(<NumericFormat value={1234} onValueChange={mockOnValueChange} thousandSeparator />); | ||
| expect(mockOnValueChange.mock.lastCall[0]).toEqual({ | ||
| formattedValue: '1,234', | ||
| value: '1234', | ||
| floatValue: 1234, | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This doesn't seem to be a correct test, why should onValueChange not be called when the input goes from a NaN value to a meaningful value? I have tried to capture this, and further scenarios in the test I have added below, please let me know if this should be adjusted or kept instead of being removed.
|
@s-yadav please check whenever you can, this is quite an intrusive bug. The fix seems to work well based on manual testing done both in the repo's example and in the code base I'm using in this library. Also, all unit tests are passing. |
|
I noticed one case that the fix isn't covering yet. This still needs more work |
|
Hey @rfviolato , this check was added to avoid double rendering, basically if the parent is controlling the state, and the state is not changed from the rnf, then don't call it to avoid possible double render from the parent setState on onValueChange. But I agree, the behaviour of onValueChange is inconsistent and makes it unreliable to put some logic, and lib shouldn't try to optimize for things happening from parent. Let's go with your fix. What's the case is not covered by the fix? |
|
@s-yadav Thanks for the reply. Sorry that was a false negative, I've been using a
Nice! 🚀 Since this issue was affecting a feature my team is developing in the code base we maintain, let me run a few more tests today, and I'll get back to you to give you the thumbs up, then we could proceed with the fix. |
|
@s-yadav We have been testing the fix and we have also been developing on top of it and it has fixed our issues with |
|
Yes, lets merge this, will make it part of next release. |
|
@s-yadav Great! When are you panning on releasing it? |
|
@rfviolato this is released on 5.4.0 |
|
Thanks @s-yadav ! |
Describe the issue/change
onValueChangeis not called for values with 3 or less characters.The problem is better described in this issue's discussion.
Add CodeSandbox link to illustrate the issue (If applicable)
https://codesandbox.io/p/sandbox/numeric-number-format-on-value-change-bug-qrgj2h?file=%2Fsrc%2FApp.tsx
Describe specs for failing cases if this is an issue (If applicable)
I have removed one of the tests because it doesn't seem to be very meaningful. In the PR's diffs, I have elaborated it in further detail.
Describe the changes proposed/implemented in this PR
This pr aims to fix the issue by simplifying the
useEffect's condition which is responsible for ensuringonValueChange's callback is invoked.Link Github issue if this PR solved an existing issue
#812
Example usage (If applicable)
n/a
Screenshot (If applicable)
n/a
Please check which browsers were used for testing