-
Notifications
You must be signed in to change notification settings - Fork 6
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: masking inputs on change when maskAllInputs:false
#61
Conversation
Since `maskInputSelector` is a new configuration item, we were not handling the case for input change when `maskAllInputs:false`. Before, input masking was only done via `maskInputOptions` and `maskAllInputs`.
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.
Makes sense to me.
l: Since this is a fix with a test rather than just a test, let's just update the title/commit message before merging, wdyt?
maskAllInputs:false
maskAllInputs:false
…olved before attempting to mask
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.
@Lms24 I made some additional changes if you could take another look and gimme your thoughts?
hasInputMaskOptions({ | ||
maskInputOptions, | ||
maskInputSelector, | ||
tagName: (target as HTMLElement).tagName, | ||
type, | ||
}) |
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.
Previous PR I had removed this condition, but that meant it was calling maskInputValue
on every change, so hasInputMaskOptions
is called before maskInputValue
. Worst case it will get called twice (it's also called in maskInputValue
to determine if it needs masking), but hasInputMaskOptions
avoids calling el.matches()/closest()
which is a bit costlier than the O(1) lookups in hasInputMaskOptions
.
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.
That makes sense to me
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 overall, just had a question
hasInputMaskOptions({ | ||
maskInputOptions, | ||
maskInputSelector, | ||
tagName: (target as HTMLElement).tagName, | ||
type, | ||
}) |
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.
That makes sense to me
* Check `maskInputOptions` if the element, based on tag name and `type` attribute, should be masked. | ||
* If `<input>` has no `type`, default to using `type="text"`. | ||
*/ | ||
function isInputTypeMasked({ |
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.
I'm probably missing something but why does this function not simply return a boolean? I checked usages and it seems like it's only used in if conditions anyway. I guess we wouldn't need the interface then either.
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.
Hmm? It does return a boolean... are the parens throwing you off? (prettier added them)
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.
Lol, sorry - it does of course. Yup, mistook the param type as the return type. Weekend here I come 😅
/** | ||
* Determine if there are masking options configured and if `maskInputValue` needs to be called | ||
*/ | ||
export function hasInputMaskOptions({ |
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.
same question here
Since
maskInputSelector
is a new configuration item, we were not handling the case for input change whenmaskAllInputs:false
. Before, input masking was only done viamaskInputOptions
andmaskAllInputs
.