-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
#448: Paths with events #692
Conversation
self.assertEqual(response[3]['value'], 1) | ||
|
||
def test_autocapture_paths(self): |
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.
opted to keep this one brief because the logic is tested above
waiting on #729 |
@timgl Can be re-reviewed. Found something funny happening with the icon alignment. Seems like the dropdown icons get un-aligned but other antd icons are aligned with the reverse 0.125em added in previous PR |
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 think this is pretty good!
RE the icons being weird, might be a case of merging in master again as it is fixed there.
With autocapture there are a lot of events that are missing the text. One thing that might be missing is going up to tree. So if you have something like this:
<a href='blabla'>
<span>some text</span>
</a>
I think currently this wouldn't show up.
I also do think it'd be useful if you could somehow click it and show the last 10 events, or maybe show the entire DOM tree. It's hard to get the context of what it means. This might be scope creep for this PR though.
RE: missing text Did a little digging and found some strange elements. These elements both appear on the /login path but somehow a div that looks like an input field is a lowest order element. I think these are the elements being picked up in the paths. Any idea why they're being saved like this and how to filter around this? |
The way posthog-js works is it adds a listener to these elements: Any click/submission on any of this saves the entire tree. In the above case, the "form" was clicked, though the top level element in this case was the div as this person mis-clicked. The way to solve this is by looking for the first element that belongs to any of those "useful elements", then grabbing the text all the way up the chain. In this case, there's no text but it should show " click" |
Got it. Is there anything else related to going up the tree other than finding the minimum order element in the element group? If the SQL I included is querying as intended it should be returning the lowest order element (top most element) in the group so those tags in the picture might just have no associated text. Will check for correctness of query. As for providing more details, I'll take a look at how we can display more details/more of the tree for ambiguous elements. |
@timgl Full DOM display should be good to review and show that the elements even without text are the top of the tree. I opted to use a separate component to show the DOM tree because I dont think it's possible to render in a jsx component in the sankey. One thing not sparking joy is that it can't autoscroll to the top to show elements if you click on a tag because of the layout component we're using. Can investigate this further in a separate issue as it might require a bit of refactoring. |
Looks great! I would suggest putting those HTML tags in a Modal instead if you can't do scroll by top. It really confused me when I kept clicking and nothing kept happening 😄 |
@timgl ready for another look with modals |
Looks great! |
Addresses #448
Changes
Checklist