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

Saved insights creation flow #5702

Merged
merged 31 commits into from
Sep 17, 2021
Merged

Saved insights creation flow #5702

merged 31 commits into from
Sep 17, 2021

Conversation

liyiy
Copy link
Contributor

@liyiy liyiy commented Aug 24, 2021

Changes

Please describe.
If this affects the frontend, include screenshots.

Connecting "create a new insight" to the saved insights page. Also updates to how insights get edited/saved.

To test, either have "dashboard_collaboration" turned on or set to "true" in the code

Screen Shot 2021-08-27 at 10 04 07 AM

Screen.Recording.2021-08-27.at.10.02.14.AM.mov
  • New main nav menu item added for "exploratory" mode and insights updated to saved insights. By default the user is directed to the "Explore" page
Screen.Recording.2021-09-14.at.10.24.50.AM.mov

Checklist

  • All querysets/queries filter by Organization, by Team, and by User
  • Django backend tests
  • Jest frontend tests
  • Cypress end-to-end tests
  • Migrations are safe to run at scale (e.g. PostHog Cloud) – present proof if not obvious
  • New/changed UI is decent on smartphones (viewport width around 360px)

@timgl timgl temporarily deployed to posthog-pr-5702 August 24, 2021 01:11 Inactive
@liyiy liyiy mentioned this pull request Aug 25, 2021
6 tasks
@timgl timgl temporarily deployed to posthog-pr-5702 August 25, 2021 13:48 Inactive
@timgl timgl temporarily deployed to posthog-pr-5702 August 27, 2021 12:49 Inactive
@timgl timgl temporarily deployed to posthog-pr-5702 August 27, 2021 13:56 Inactive
@timgl timgl temporarily deployed to posthog-pr-5702 August 28, 2021 19:56 Inactive
@timgl timgl temporarily deployed to posthog-pr-5702 August 28, 2021 21:12 Inactive
@liyiy liyiy marked this pull request as ready for review August 28, 2021 21:21
@PostHog PostHog deleted a comment from posthog-bot Sep 2, 2021
@liyiy liyiy force-pushed the saved-insights-creation-flow branch from 818e8a4 to d6ed68c Compare September 6, 2021 12:54
@timgl timgl temporarily deployed to posthog-pr-5702 September 14, 2021 14:28 Inactive
onClick={() =>
setInsightMode({ mode: ItemMode.Edit, source: InsightEventSource.InsightHeader })
<>
{insightMode === ItemMode.View ? (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFTER CONVENING WITH CHRIS, I believe this flow should make a lot more sense now. Basically we are dividing the insight flow into a view versus edit mode. When we click on "explore" (what used to be "insights"), we are sent straight to the insight creation edit mode. However when we open up a saved insight or dashboard item, we are sent to the insight's view mode (where there is an option to intentionally edit). In addition to that, whenever we're "editing" an insight now, we are editing the same insight per editing "session". Previously we were creating a brand new insight for every tab change, filter update, etc, but now we are updating the same one instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen.Recording.2021-09-14.at.10.24.50.AM.mov

@timgl timgl temporarily deployed to posthog-pr-5702 September 14, 2021 14:47 Inactive
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a few conflicts with dashboard logic, otherwise looks really good and can't wait to get this in! :)

Comment on lines 577 to 580
if (!insightLogic.values.insight.id) {
actions.createInsight(values.filters)
}
insightLogic.actions.updateInsightFilters(values.filters)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actions.createInsight is actually insightHistoryLogic.actions.createInsight, which calls api.create('api/insight'). The problem is that funnelLogic is also mounted for each dashboard item, so this makes a lot of mess when loading a dashboard:

image

You can just if (!props.dashboardItemId) all the code that touches insightLogic as a quick solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!!! I've feature flagged this (since it's a saved insights flow functionality) and also made sure to update only when an insight actually exists to be updated

frontend/src/scenes/paths/pathsLogic.ts Outdated Show resolved Hide resolved
frontend/src/scenes/retention/retentionTableLogic.ts Outdated Show resolved Hide resolved
frontend/src/scenes/trends/trendsLogic.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@paolodamico paolodamico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Product-based review, as @mariusandra has reviewed the code. As a preamble, this is great so far!

No feature flag

  1. When creating a new insight, this is the view you see. I found three things confusing: a) the "Insight #{x}", if this is the insight ID I think it'll cause confusion on Cloud, b) it's confusing to see "Edit" when you haven't actually saved anything, c) there's two "add to dashboard" buttons.
  2. The funnels page still has a save button.

With enabled feature flag

  1. UX-wise I think it's confusing that you can have the same insight named differently when saving on a dashboard than when saving it standalone (as when saving you get asked for a new insight name)
  2. Clicking New Insight -> User Paths is broken
  3. I'd remove the created by filter when on the "Your insights" tab
  4. Some insight icons in the type of insight filter seem confusing as we use them for other things (e.g. sessions, stickiness)
  5. Currently there's no way to access the /saved_insights page
  6. In a fresh installation I had a saved insight created as "Funnel x3" but it was actually a retention graph.
  7. There's some weird positioning of elements in some rows.

Screenshot 2021-09-16 at 6 07 24 PM

10. It's not obvious that you can click the 3 dots menu, I would change the color to `$primary`. 11. The more filter box does not have the appropriate rounding and shadowing.

12. I would add the relevant icons to the dropdown menu actions (like we have in the dashboard page).

@timgl timgl temporarily deployed to posthog-pr-5702 September 16, 2021 16:12 Inactive
@timgl timgl temporarily deployed to posthog-pr-5702 September 16, 2021 18:35 Inactive
@timgl timgl temporarily deployed to posthog-pr-5702 September 16, 2021 18:36 Inactive
@liyiy
Copy link
Contributor Author

liyiy commented Sep 16, 2021

Addressed @mariusandra's comments and most of @paolodamico's (couldn't reproduce some of the issues). Will be leaving some of the UX improvements for a future PR so we can try to get this in :D

@paolodamico
Copy link
Contributor

Makes sense @liyiy, I agree with merging and leaving UI/UX improvements for a subsequent PR. I'll just defer to @mariusandra for the final 👌 as he reviewed the code

Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, unfortunately I can't approve it. The "loadResultsSuccess" listeners are still wrong and don't explicitly take into account that none of this code should be run if we're on a dashboard. Please keep the old if (!props.dashboardItemId) { code and move the new logic inside that.

@timgl timgl temporarily deployed to posthog-pr-5702 September 17, 2021 12:24 Inactive
@mariusandra mariusandra merged commit 3f9a6bf into master Sep 17, 2021
@mariusandra mariusandra deleted the saved-insights-creation-flow branch September 17, 2021 12:36
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.

4 participants