fix: fewer moving parts more like safe text #590
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pauldambra
commented
Mar 20, 2023
| * @param {strings} [] - strings to join | ||
| * @returns {string} - joined strings | ||
| */ | ||
| export function concatenateStringsWithSpace(strings: string[]): string { |
There was a problem hiding this comment.
we're always concatenating exactly two strings so we can template them together instead
|
Size Change: -507 B (0%) Total Size: 2.29 MB
ℹ️ View Unchanged
|
pauldambra
commented
Mar 20, 2023
| if (target && target.nodeType === 1 && target.children?.length > 0) { | ||
| for (const child of target.children) { | ||
| if (child && child.nodeType === 1 && child.tagName?.toLowerCase() === 'span') { | ||
| if (target && target.childNodes && target.childNodes.length) { |
There was a problem hiding this comment.
It doesn't actually make a tonne of sense to use childNodes since that will include comment and text nodes as well as elements. But none of this makes sense and we don't get (or don't notice) errors from getSafeText so I wanted to shift this to be as similar as possible.
It iterates childNodes with _each so that's what I've changed this to do
We check target.childNodes && target.childNodes.length so it is implicit that Element is not a text or comment node. We then check that the child has a tagName === span, so it is implicit there too
pauldambra
commented
Mar 20, 2023
| @@ -78,7 +78,7 @@ const autocapture = { | |||
| tag_name: tag_name, | |||
| } | |||
| if (autocaptureCompatibleElements.indexOf(tag_name) > -1 && !maskText) { | |||
| if (elem.tagName.toLowerCase() === 'a' || elem.tagName.toLowerCase() === 'button') { | |||
| if (tag_name.toLowerCase() === 'a' || tag_name.toLowerCase() === 'button') { | |||
There was a problem hiding this comment.
elem.tagName is captured as tab_name above. Using it here just for tidiness
pauldambra
changed the title
raquelmsmith
approved these changes
Mar 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
I'm waiting for another build to finish and my brain was chewing at this...
I thought I'd try making it more like
getSafeTextand remove as many moving parts as I could think ofIt's very much a "poke it with a stick and see what happens change"