Skip to content
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

Merged
merged 6 commits into from
Feb 24, 2023

Conversation

billyvg
Copy link
Member

@billyvg billyvg commented Feb 22, 2023

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.

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`.
Copy link
Member

@Lms24 Lms24 left a 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?

@billyvg billyvg changed the title test: Fix masking inputs on change when maskAllInputs:false fix: masking inputs on change when maskAllInputs:false Feb 23, 2023
Copy link
Member Author

@billyvg billyvg left a 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?

Comment on lines +374 to +379
hasInputMaskOptions({
maskInputOptions,
maskInputSelector,
tagName: (target as HTMLElement).tagName,
type,
})
Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member

@Lms24 Lms24 left a 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

Comment on lines +374 to +379
hasInputMaskOptions({
maskInputOptions,
maskInputSelector,
tagName: (target as HTMLElement).tagName,
type,
})
Copy link
Member

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({
Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member

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({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question here

@billyvg billyvg merged commit 6db7ad8 into sentry-v1 Feb 24, 2023
@billyvg billyvg deleted the test-mask-all-text-false-input-mutation branch February 24, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants