-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Clear input value on Command + Delete for NumberField/ColorField #2005
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
Conversation
Build successful! 🎉 |
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.
Mind adding a test here as well? https://github.com/adobe/react-spectrum/blob/main/packages/@react-spectrum/numberfield/test/NumberField.test.js#L1915
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.
Thanks for adding these to our tests! Some manual testing notes:
Tested an old build
I don't think this is working correctly in Safari? I can't see CMD + Delete doing anything.
I think it's also not intended to be able to delete partial currency symbols, see https://reactspectrum.blob.core.windows.net/reactspectrum/50233b6d2f4b110c70f7a2badd5b76e61b1d76bc/storybook/index.html?path=/story/numberfield--currency-switcher
Set the Currency Display to Code, place the cursor in between two of the currency symbol letters, press Delete, nothing happens. Press CMD + Delete, it deletes a portion of the code.
What does state.validate return? That's the check to see if we should block the event and I'm surprised it's not blocking it while your test indicates it is.
https://github.com/adobe/react-spectrum/pull/2005/files#diff-0d27e9828095cc31dbe37a372f0dfc33464dfa4c1dd426d7952a3a6e8df28e25R99
Build successful! 🎉 |
Build successful! 🎉 |
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.
Tested across Chrome/FF/Safari, lgtm
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.
LGTM
Closes #1877
✅ Pull Request Checklist:
📝 Test Instructions:
For components that use useFormattedTextField (i.e. NumberField and ColorField):
🧢 Your Project: