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

#448: Paths with events #692

Merged
merged 17 commits into from
May 18, 2020
Merged

#448: Paths with events #692

merged 17 commits into from
May 18, 2020

Conversation

EDsCODE
Copy link
Member

@EDsCODE EDsCODE commented Apr 29, 2020

Addresses #448

Changes

  • adds options on paths page to allow different type of paths beyond pageviews

Checklist

  • All querysets/queries filter by Team (if applicable)
  • Backend tests (if applicable)

@timgl timgl temporarily deployed to posthog-448-nkn72gvwklln3hb47f April 29, 2020 23:58 Inactive
posthog/api/paths.py Outdated Show resolved Hide resolved
@timgl timgl temporarily deployed to posthog-448-nkn72gvwklln3hb47f May 4, 2020 14:12 Inactive
@timgl timgl temporarily deployed to posthog-448-nkn72gvwklln3hb47f May 4, 2020 15:26 Inactive
@EDsCODE EDsCODE requested a review from timgl May 4, 2020 15:30
@EDsCODE EDsCODE marked this pull request as ready for review May 4, 2020 15:31
self.assertEqual(response[3]['value'], 1)

def test_autocapture_paths(self):
Copy link
Member Author

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

@EDsCODE
Copy link
Member Author

EDsCODE commented May 6, 2020

waiting on #729

@timgl timgl temporarily deployed to posthog-448-nkn72gvwklln3hb47f May 12, 2020 02:00 Inactive
@EDsCODE
Copy link
Member Author

EDsCODE commented May 12, 2020

@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

Screen Shot 2020-05-11 at 9 59 05 PM

Copy link
Collaborator

@timgl timgl left a 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.

image
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.

@EDsCODE
Copy link
Member Author

EDsCODE commented May 14, 2020

RE: missing text
The query I added ensures that always the lowest order element is the one that's "focused on" when creating the element paths.

Screen Shot 2020-05-14 at 10 12 02 AM

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?

@timgl
Copy link
Collaborator

timgl commented May 15, 2020

The way posthog-js works is it adds a listener to these elements: ['a', 'button', 'form', 'input', 'select', 'textarea', 'label']

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"

@EDsCODE
Copy link
Member Author

EDsCODE commented May 18, 2020

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.

@EDsCODE
Copy link
Member Author

EDsCODE commented May 18, 2020

@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.

@timgl
Copy link
Collaborator

timgl commented May 18, 2020

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 😄

@EDsCODE
Copy link
Member Author

EDsCODE commented May 18, 2020

@timgl ready for another look with modals

@timgl timgl merged commit 0facaeb into master May 18, 2020
@timgl timgl deleted the #448 branch May 18, 2020 21:21
@timgl
Copy link
Collaborator

timgl commented May 18, 2020

Looks great!

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