-
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: create experiment from a funnel #27473
Conversation
📸 UI snapshots have been updated99 snapshot changes in total. 0 added, 99 modified, 0 deleted:
Triggered by this commit. |
Size Change: +33 B (0%) Total Size: 1.16 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated97 snapshot changes in total. 0 added, 97 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated28 snapshot changes in total. 0 added, 28 modified, 0 deleted:
Triggered by this commit. |
@jurajmajerik I've stripped out any properties that aren't currently available in the experiments UI to avoid a situation where the user can't change the metric |
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.
Cool feature, thanks for taking this on! 🙂 A few things to address:
General feedback: when adding a UI feature, please add screenshots and/or a Loom, makes it much easier to see how the thing should be tested. |
Why not add it for both right away? The code to do this is already there, so should be straightforward. Also, I see that you tested for the Trends option too?
|
Currently the IMO, whenever the intention is to create an experiment from an insight, but we fail to load the insight, we should throw. Otherwise the experiment gets saved without the metric which is poor user experience. |
I personally like the position of this button, as it's prominent and will likely drive adoption for experiments. But I'd check with @PostHog/team-product-analytics if it's a good idea to put it there. Perhaps the intent of this panel is to only show properties related to the chart, so better to double check with them. |
@jurajmajerik Addressed your comments here, thanks for these. Also added a Loom showing functionality locally: loom.com/share/ba32c35e3a794e08ab35eb28f8a80e56 |
Thanks @joshsny, working great! A few more comments:
|
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.
Awesome, this looks great :)
I'll fall to @jurajmajerik for approval - I like his suggestions for the tooltip and banner :)
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
@jurajmajerik thanks for these suggestions!
I agree, have refactored the logic to pass metric instead of rely on insight.
Done
Done, great suggestion. I don't love the UX for this page with having the metric on the next page, but it's out of scope of this PR to try and change that so the banner at least communicates something to the user.
Was waiting on this to merge, since that's done now I've added tracking for the cross sell. Updated loom demo: https://www.loom.com/share/42aa7fcc2136495fb3302cdfdd58b19c?sid=dcad2565-d000-42ae-a23a-125ef560e674 |
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.
Nice work! 🙌
📸 UI snapshots have been updated24 snapshot changes in total. 0 added, 24 modified, 0 deleted:
Triggered by this commit. |
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Problem
If a user creates an insight to track a metric, it's likely they want to improve that metric if they can. This supports creating an experiment from an insight. Right now we surface this option for a funnel only, and will track engagement.
Changes
Loom: https://www.loom.com/share/ba32c35e3a794e08ab35eb28f8a80e56
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Tested for 3 scenarios: