-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix bug in X-Mask where user input that matched the non wildcard value were being stripped #4253
Changes from all commits
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 | ||||
---|---|---|---|---|---|---|
|
@@ -11,6 +11,8 @@ test('strip-down functionality', async () => { | |||||
expect(stripDown('(999) 999-9999', '716 2256108')).toEqual('7162256108') | ||||||
expect(stripDown('(999) 999-9999', '(716) 2256108')).toEqual('7162256108') | ||||||
expect(stripDown('(999) 999-9999', '(716) 2-25--6108')).toEqual('7162256108') | ||||||
expect(stripDown('+1 (999) 999-9999', '7162256108')).toEqual('7162256108') | ||||||
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
this would be a more accurate test to what you wanted to "fix". Which will likely fail, outputting 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. Yes you are correct that this solution does not handle that latter case, I think both your suggestion and what I introduced would be good introductions to the spec and would mimic behavior of other masking tools (e.g. maska https://codepen.io/Robert-Marney/pen/LYoLVRb) In the pen above you can see the mask which is equivalent to the test case can handle both types of user input (e.g. a direct mask format as well as the numbers represented by the wildcard fields) 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. Just to clarify, which bug qre you trying to fix here? The example you posted (maska) behaves exactly as x-mask. 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. @SimoTod - The bug is the application of autocomplete values 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. Ok, that is the first time you mention it. Are we saying that the other plugins work differently depending on you entering the number vs copying vs using autoconplete? In that case, it looks like x-mask does a better job at staying consistent. It also sounds like it would be a lot of work to make it work the way you like. If it's causing pains in your application, you can maybe disable the autocomplete? 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. |
||||||
expect(stripDown('ABC (999) 999-9999', '7162256108')).toEqual('7162256108') | ||||||
}) | ||||||
|
||||||
test('formatMoney functionality', async () => { | ||||||
|
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.
This can never evaluate to true.
It's saying "if these match and if they also don't match" which is impossible.
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.
Hi @ekwoka - Thanks for the quick feedback.
I disagree, note that the new conditional is not using the same iterator for inputToBeStripped (we are using i not j)
From
mask.spec.js:L11
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.
Ah yes, you're right. That's hard to read.