-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: Improve path cleaning UI/UX #27997
feat: Improve path cleaning UI/UX #27997
Conversation
@robbie-c In case you're wondering where I'm going with this, I need the capability to, when looking from the toolbar, see all of the events from that page after it's been aliased, so I need whatever I'm implementing in the following PR, this one is just cleaning this up and making the feature slightly more useful |
Size Change: +123 B (+0.01%) Total Size: 1.16 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
7ca5470
to
247bfc8
Compare
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
Thanks for merging with master @robbie-c, would love a review :) |
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.
Apologies that this is a partial review - will finish the review tomorrow morning my time
Rules that you set here will be applied before wildcarding and other regex replacement if the toggle | ||
is switched on. | ||
Rules that you set here will be applied before wildcarding and other regex replacement if path | ||
cleaning is switched on in Product and Web Analytics. |
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 went through the other products to see if I could find somewhere else that used this - for some reason I thought replay did too - but it's really just these :)
<Tooltip | ||
title={ | ||
<span> | ||
Path Breakdown is more useful when path cleaning is turned on.{' '} |
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.
Aggressive copy but I like it :)
@@ -234,10 +234,33 @@ def _expr_to_compare_op( | |||
return ast.CompareOperation(op=ast.CompareOperationOp.LtEq, left=expr, right=ast.Constant(value=value)) | |||
elif operator == PropertyOperator.GTE: | |||
return ast.CompareOperation(op=ast.CompareOperationOp.GtEq, left=expr, right=ast.Constant(value=value)) | |||
elif operator == PropertyOperator.IS_CLEANED_PATH_EXACT: |
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 need to think about this a bit more, but it seems like if we were going the route of adding new PropertyOperator
s, there's a few more along similar lines that would be really useful. Obviously the negated version. Also maybe a startsWith?
My worry is that this approach would lead to an explosion of operators. Is there a good way of having this power without making it too hard for people to find it?
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.
The thing is, Im not adding it as an operator that can be selected in the UI at the moment, Im dynamically translating it to this operator when needed.
I see the benefit to creating them "for real" eventually, but it's not something we need now.
I couldnt come up with a more elegant solution, any ideas?
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.
Ah if it's not in the UI then I'm less worried, that makes sense
@@ -689,7 +706,7 @@ export const webAnalyticsLogic = kea<webAnalyticsLogicType>([ | |||
} | |||
: undefined | |||
|
|||
// the queries don't currently revenue when the conversion goal is an action | |||
// the queries don't currently include revenue when the conversion goal is an action |
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.
🙃
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.
Broadly, this looks great! I approve, and props for improving this.
I haven't had a chance to run this locally yet - but one thing I am concerned about is backwards compatability. I would like to see something that shows that this has been considered. Either:
- put it behind a FF so we can roll out slowly and revert if a customer's use case breaks
- write some tests to show that backwards compat is fine
- write a big comment explaining why everything is backwards compatible
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.
Approved but add a FF :)
Ideally we'd want a smaller button, but people aren't familar with path cleaning yet, so let's make it slightly exaggerated
This is now much cleaner, and we also have a way to test what the path will look like once cleaned
Paths displayed on Web Analytics/Web Vitals table now properly render them using the new readable format for them, assuming people use the suggested `<slug>` format In a future PR we'll enable people to filter by cleaned paths, making cleaned paths much more powerful for WA/WV
We're now allowing us to filter by cleaned path, which makes our UI slightly more powerful. If you select a cleaned path and `doPathCleaning` is set, we can see results from a specific cleaned path. The UX isn't great because it looks kinda wonky, and I'm pretending it's a `EXACT` match (which kinda is?) and you can't get into that state unless you click on a path in the UI, but it does work, so I think this is a win. Eventually once we have `$pathname` a high-level filter (rather than just another filter) we'll be able to make this behave better. Web Vitals is coming in the next commit
Now, if path cleaning is enabled and I filter by a path that would be cleaned, we display data from the filtered path instead
You can now click on a path on web vitals path breakdown to add it to the filter
We don't need to calculate that before hand, we can simply pass the metric to the function and it knows how to handle it
We don't believe we'll need to disable this in the future, but let's keep this around as a safety switch
d27ad19
to
16c0621
Compare
FF added on 16c0621, will merge this with 100% turned on, will act as a kill-switch, thanks! |
📸 UI snapshots have been updated6 snapshot changes in total. 0 added, 6 modified, 0 deleted:
Triggered by this commit. |
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Robbie <robbie.coomber@gmail.com>
Paths displayed on Web Analytics/Web Vitals table now properly render them using the new readable format for them, assuming people use the suggested
<slug>
formatI've also made path cleaning a top-level filter on Web Analytics because I want to push people to use it to increase adoption. Once people have them set-up, the toolbar will be a no-brainer to explore Web Vitals
We've also changed how filtering works so that if someone has path cleaning enabled and clicks in one of the cleaned paths, we'll properly display results for that filter 🎉. To make things even, we've added this feature to Web Vitals as well.