-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Reset amount on reset button click #42897
Changes from all commits
78f25f7
5a4feba
98c2c78
37af19b
4b30dd9
4c4e872
cde4e5f
c805f32
812ce37
7cd56e9
8f71b5d
057afeb
bc0b3b3
96c4aca
1f5660c
6ea1723
364708b
b454ac2
bc10692
4db9a0a
b3087ed
d5df8f5
10d3696
1b40934
5a81a36
ff59531
8042286
5a2602c
221b7a9
bf86b78
b1eda3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -88,6 +88,18 @@ type MoneyRequestAmountInputProps = { | |||||
* Autogrow input container length based on the entered text. | ||||||
*/ | ||||||
autoGrow?: boolean; | ||||||
|
||||||
/** | ||||||
* Determines whether the amount should be reset. | ||||||
*/ | ||||||
shouldResetAmount?: boolean; | ||||||
|
||||||
/** | ||||||
* Callback function triggered when the amount is reset. | ||||||
* | ||||||
* @param resetValue - A boolean indicating whether the amount should be reset. | ||||||
*/ | ||||||
onResetAmount?: (resetValue: boolean) => void; | ||||||
}; | ||||||
|
||||||
type Selection = { | ||||||
|
@@ -123,6 +135,8 @@ function MoneyRequestAmountInput( | |||||
hideFocusedState = true, | ||||||
shouldKeepUserInput = false, | ||||||
autoGrow = true, | ||||||
shouldResetAmount, | ||||||
onResetAmount, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
...props | ||||||
}: MoneyRequestAmountInputProps, | ||||||
forwardedRef: ForwardedRef<BaseTextInputRef>, | ||||||
|
@@ -202,10 +216,21 @@ function MoneyRequestAmountInput( | |||||
})); | ||||||
|
||||||
useEffect(() => { | ||||||
const frontendAmount = onFormatAmount(amount, currency); | ||||||
setCurrentAmount(frontendAmount); | ||||||
if (shouldResetAmount) { | ||||||
setSelection({ | ||||||
start: frontendAmount.length, | ||||||
end: frontendAmount.length, | ||||||
}); | ||||||
onResetAmount?.(false); | ||||||
return; | ||||||
} | ||||||
|
||||||
if ((!currency || typeof amount !== 'number' || (formatAmountOnBlur && isTextInputFocused(textInput))) ?? shouldKeepUserInput) { | ||||||
return; | ||||||
} | ||||||
const frontendAmount = onFormatAmount(amount, currency); | ||||||
|
||||||
setCurrentAmount(frontendAmount); | ||||||
|
||||||
// Only update selection if the amount prop was changed from the outside and is not the same as the current amount we just computed | ||||||
|
@@ -216,10 +241,7 @@ function MoneyRequestAmountInput( | |||||
end: frontendAmount.length, | ||||||
}); | ||||||
} | ||||||
|
||||||
// we want to re-initialize the state only when the amount changes | ||||||
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps | ||||||
}, [amount, shouldKeepUserInput]); | ||||||
}, [amount, currency, formatAmountOnBlur, shouldKeepUserInput, onFormatAmount, shouldResetAmount, onResetAmount, currentAmount]); | ||||||
Comment on lines
-219
to
+244
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey 👋 It looks like this change introduced a bug (check the revert PR description) cc @mountiny There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried to bring those lines back
and the input is working again, so a fix should be pretty easy :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Heads up we ended up reverting this PR because of the above mentioned critical issue. We opted for a revert to make sure there are no other unwanted regressions @kaushiktd can you raise a new PR and make sure to test for the input working well |
||||||
|
||||||
// Modifies the amount to match the decimals for changed currency. | ||||||
useEffect(() => { | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add comments to both of these props please?