-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Cursor position in FormattedInput #1257
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1257 +/- ##
=======================================
Coverage 93.46% 93.46%
=======================================
Files 114 114
Lines 4404 4404
=======================================
Hits 4116 4116
Misses 288 288
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
@priyang12 Good job on this PR! There is an edge case I noticed, when you edit a lengthy number (in the middle) which has a repeated number in it, eg.,100000000, the cursor position is not placed accurately. However, before handling this, I am re-thinking the whole input. Here's the relevant discussion: #1268. I'm placing the review and merge on hold until that discussion is resolved by the core team. |
@pavish ok thanks for the update. |
The discussion #1268 is resolved. @seancolsen I'm assigning you as the reviewer for this PR, since you've been the one working on the NumberInput component. |
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.
@priyang12 Thanks again for this PR.
For the most part, your work has improved the behavior of the input, so I'm merging this PR. However, as @pavish mentioned, there are some additional edge cases that we'll still need to handle. To track the work of handling those additional edge cases, I've opened #1284.
I pushed a commit to this PR which restructures your code as follows:
- I renamed the function
setCursorToPostion
togetCursorPositionAfterReformat
. That fixes the spelling error in "position" and makes the name more specific. - I made the function into a pure function and moved it to a utils file so that it's easier to test with unit tests.
- I changed the argument structure to use named arguments with names that pertain to the purpose of the function instead of the purpose of the component. This increases readability when calling the function and reduces the risk of calling the function with the arguments mixed up.
- I removed the TypeScript type
any
from within the function. We don't need to use it here. We should only useany
as a last resort. - I made some other small naming changes within the function to increase readability.
- I wrote a unit test with several test cases that pass. And I added more test cases that fail (with those test cases commented out.)
- I removed the logic which checks to see if the last character is a number. We shouldn't need to do that. The process of setting the cursor position should work regardless of the characters in the text. One important reason is that the
FormattedInput
component is a general purpose component which serves as a building block for other components. So far, we only haveNumberInput
. But we may someday have others, likePhoneNumberInput
orDateInput
orCreditCardNumberInput
. Further, withinNumberInput
specifically, sometimes the last character of the number isn't a digit. For example, we need to gracefully deal with an input of12.
.
Fixes #1248
Technical details
Corrected Cursor Position using Diff package by creating a function.
Screenshots
Screen.Recording.2022-03-31.at.7.30.11.PM.mov
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin