Repository navigation
fix: spans inside buttons - #563
Conversation
|
Size Change: +2.53 kB (0%) Total Size: 2.28 MB
ℹ️ View Unchanged
|
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
looks good to me, I've not used yalc to run it, but I can see you edited those instructions so I'm assuming you did 🙌
I think that anyone who was relying on us not detecting spans can create an action to unify the old and new autocapture event if they need to. But this behaviour seems more correct to me so 👍
|
I bet we could go spelunking for insights that use autocapture where those elements contain buttons that contain spans. But that feels like overkill, let's drop a heads-up in the community slack when we release it and can help anyone that needs the help |
|
I realized that if someone clicked the |
| if (target.children?.length > 0) { | ||
| for (const child of target?.children) { |
There was a problem hiding this comment.
| if (target.children?.length > 0) { | |
| for (const child of target?.children) { | |
| if ((target?.children || []).length > 0) { | |
| for (const child of target.children) { |
nit picking, since we've got target.children on the line above we don't need target?.children here.
If target is undefined it'll fail on line 341.
(sorry, if my last nit-pick here dig a hole for you with IE 😅)
There was a problem hiding this comment.
It's weird because that change you suggested initially passed the IE test, but then failed subsequently, but reverting it made it pass again 🤷♀️ I'll update kinda as you suggest here (I'd argue that the way I did it is overall more readable esp since we can't call the func without a target) but if it fails after I make the commit it'll be your fault 😛
There was a problem hiding this comment.
🤣
(I'd argue that the way I did it is overall more readable)
So, there meaning
if (target.children?.length > 0)
vs.
if ((target?.children || []).length > 0)
Yep, we're fighting JS (and I did say I was nit-picking so feel free to ignore)
Even though TS says target can't be undefined JS won't care so it can be. Which would mean target.children could fail. Before you get to the target.children?.length. A little over cautious but the SDK is a pain for hitting all the possible errors.
I guess you could have if(target && target.children?.length > 0) instead.
If it's not undefined then the spec says we definitely get children... But, JS is fun so I won't quibble over the children?.length not needing the elvis operator since it doesn't cost us anything.
(Going into detail in case I'm being stupid and this helps you figure out whether to ignore me :))
There was a problem hiding this comment.
No, you're not being stupid, I guess I'm just being overly trusting of typescript haha. I hit the merge button too quick. I'll open another PR and it can go out with the next version.
Changes
Fixes PostHog/posthog#14387
If an anchor or button element had its text nested inside a
<span>(eg<button href="#"><span>Some Text</span></button>then autocapture would not assign that text to the button when the button click was captured. So if in the example there, someone clicked on the button element but not directly on the span (because padding or something) then it would capture the button click, but someone's action forautocapture text contains Some Textwould not get triggered.With this change we now iterate through any children inside
aandbuttonelements, looking forspans, pull the text out of those spans, and assign them to the button text property.❓ I'm not sure how impactful this change will be... I believe that this would be the expected behavior for autocapture, but it's possible that it could mess up some peoples' metrics.... How should we go about deciding how to release this?
...
Checklist