-
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
feat(product analytics): Remove insight exit pop up #26878
Conversation
@@ -671,6 +683,13 @@ export function SavedInsights(): JSX.Element { | |||
<SavedInsightsEmptyState /> | |||
) : ( | |||
<> | |||
{draftQuery?.query && ( | |||
<div className="text-muted-alt mb-4"> |
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.
Might make sense to show this on the new insight page as well?
when someone tries to create a new insight again (clicking the + button without going to the saved insights page) they might want to go back to the previous state they had
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.
Great work on this! Such a massive UX improvement for users by removing the popup. Love how we can get back to the unsaved insight. Just one thing I noticed is that when you're adding a new insight from a dashboard (or when you click back on the + sign) you are brought back to the unsaved insight.
Screen.Recording.2024-12-13.at.22.10.42.mov
Think it might be better to give the user an option to revert to the unsaved state instead of starting off from it? Especially in case of adding insights on dashboards where the unsaved insight might have been quite different + if unsaved insight had a lot of filters, the user will need to remove everything to get the blank slate to start from. I might be wrong here, so will let others chime in as well :)
Looking great otherwise!
A thought about users with multiple teams (soon to be called environments): for users with multiple teams, we would want to scope these localstorage queries to only show up in a given team |
cd312c7
to
724cc7e
Compare
Size Change: +1.61 kB (+0.14%) Total Size: 1.11 MB
|
return ( | ||
<div className="text-muted-alt mb-4"> | ||
You have an unsaved insight from {new Date(draftQuery.timestamp).toLocaleString()}.{' '} | ||
<Link to={urls.insightNew(undefined, null, draftQuery.query)}>Click here</Link> to view 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.
small nit: how do you feel about changing this to say "click here to load it" if you are already on the insight editor?
I tried this out and it seems great. I did struggle with loading in the mental model of how the unsaved insight interacts with edit mode. If you have an unsaved insight, and then browse to any insight and click edit, and then make any changes there, it overrides your unsaved insight. If you then go and "view" your unsaved insight, it doesn't take you back to edit mode for the saved insight, but takes you to a generic edit mode for a new insight. If you edit it and save, you then get a modified version of the insight you went to edit. I could see this being confusing. I don't have a slam dunk answer here to make it simple, but potentially we could make the "unsaved insight" feature only applies to new insights and we always show the save modal if you are in "edit" mode for an existing insight and try to navigate away. I could see a more complex interaction with editing saved insights in the future, but I don't want to scope creep this too much. If you have a better model for how this interaction could work, open to hearing it. |
…posthog into feat/remove-confirm-leave-alert
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
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.
This looks great!
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
…posthog into feat/remove-confirm-leave-alert
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
Problem
When exploring with an new insight or editing an existing insight users are frustrated with the web alert pop up to confirm leaving the page. Also to share an explore you first have to save it and then share the url.
Changes
Removed popup from temporary insights or insights that are being edited.
Instead saving the query to both the url and local storage
Allow user to return to temp insights via a new button and via url
Example url
http://localhost:8000/project/1/insights/new#q=%7B%22kind%22%3A%22InsightVizNode%22%2C%22source%22%3A%7B%22kind%22%3A%22TrendsQuery%22%2C%22series%22%3A%5B%7B%22kind%22%3A%22EventsNode%22%2C%22event%22%3A%22%24pageview%22%2C%22name%22%3A%22%24pageview%22%2C%22math%22%3A%22first_time_for_user%22%7D%5D%2C%22trendsFilter%22%3A%7B%7D%7D%7D%20
How did you test this code
Todo