Saved insights default 2 #5616

liyiy · 2021-08-17T13:10:27Z

Changes

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

Feature flag 3408-saved-insights should be on for this to work. After that, navigate to / and it should take you to /saved_insights automatically

Updates to the search/filters bar, card layout, and insight action dropdowns

Screen.Recording.2021-08-19.at.1.11.50.AM.mov

Screen.Recording.2021-08-21.at.8.15.54.PM.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)

liyiy · 2021-08-17T17:13:35Z

@clarkus What kind of dropdown options do you think makes sense for last modified?

clarkus · 2021-08-17T17:31:13Z

@liyiy I think we could mimic the time range options shown for visualizations. There are a lot of options here, but it does set some precedent for the presets users might expect. Let's start with this. We can update later after we see how it's being used.

clarkus · 2021-08-19T20:28:58Z

Here's a quick update that aligns the dashboard item menus to the ones used on insights. https://www.figma.com/file/gQBj9YnNgD8YW4nBwCVLZf/PostHog-App?node-id=3164%3A39543

paolodamico · 2021-08-24T03:29:32Z

Pretty swamped this week, @mariusandra @alexkim205 would you be able to help with a review here?

alexkim205 · 2021-08-24T04:45:23Z

frontend/src/scenes/dashboard/DashboardItem.tsx

@@ -67,7 +52,6 @@ export type DisplayedType = ChartDisplayType | 'RetentionContainer'
 interface DisplayProps {
    className: string
    element: (props: any) => JSX.Element | null
-    icon: (props: any) => JSX.Element | null


Hmm, will removing icons here be consistent with how we want to use DashboardItems in InsightHistoryPanel and DashboardItems?

I think we're getting rid of insight history panel and yes, it should be gone in the dashboard items I believe under the mocks chris posted?

Yeah I've been dropping icons from the menus. I think we're overusing them in a few places. The exceptions for inline icons so far have been + icons as prefixes in buttons.

frontend/src/scenes/saved-insights/SavedInsights.tsx

alexkim205 · 2021-08-24T05:04:38Z

frontend/src/scenes/saved-insights/savedInsightsLogic.ts

+        },
+        setCreatedBy: () => {
+            actions.loadInsights()
+        },


Kind of torn if it's worth the refactor to put all these parameters (tab, search term, dates, type, etc.) under a source of truth filters like we have in our insight logics. It would make for less boilerplate code and could persist these options in the url (which is especially helpful if the user wants to navigate back to their saved_insights after clicking an insight row). @mariusandra what are your thoughts?

It would indeed be great to persist these variables in the URL... and that's indeed easier to achieve if you have one filters reducer, versus multiple other reducers, though this can work as well.

posthog/api/insight.py

alexkim205 · 2021-08-24T05:09:12Z

Hey @liyiy, looking nice! Left some inline comments + some bugs I ran into below:

Not sure if my CH setup is being uncooperative, but I get stuck in a reload loop when I try to spin this up with yarn start-ch-dev and navigate to /saved_insights:

Screen.Recording.2021-08-23.at.9.43.38.PM.mov

Counts start with 1 even if insights don't exist:

liyiy · 2021-08-25T01:59:27Z

@alexkim205 hmm how are you accessing that saved_insights endpoint? The feature flag needs to be on for it to work

mariusandra

I tested this (and the relevant part under insights), and found some mostly-UX problems.

a) It doesn't want to change the name, and b) clicking on "description" seems focuses the "name" field
Afterwards updating the filter loses data
The save button and its connected "save to dashboard" flow seems off here:
It wasn't obvious that to test this, I had to open the / path, which would take me to the saved insights page. This would be nice to include in the PR description for people who want to test (unrelated pet peeve of mine: screencasts that just omit the URL bar :D). There was also nothing I could click to open this page.
I can filter by types, but in the "list" view I don't see what types are in the list. Perhaps icons to the rescue? :D
The "last modified" and "created by" columns just sort the current page, not the entire list:
With the "..." icon so close to the name, it doesn't feel like a row-level menu, but more like a user-level menu.
In the card view I lose the information about who made the insight. The old "home" view does contain this information for example:
Let the user edit the name when renaming:

mariusandra · 2021-08-25T10:59:33Z

frontend/src/scenes/saved-insights/SavedInsights.scss

+    .dashboard-item {
+        border: 1px solid transparent;
+        padding-bottom: 10px;
+        cursor: pointer;
+
+        .dashboard-item-container:not(.funnel) .dashboard-item-content {
+            transform: scale(0.333);
+            transform-origin: top left;
+            overflow: inherit;
+            position: inherit;
+            & > div {
+                height: 300% !important;
+                width: 300% !important;
+            }
+        }
+        .dashboard-item-header {
+            cursor: pointer !important;
+        }
+
+        .svg-funnel-js__label * {
+            font-size: 8px;
+        }
+        &:hover {
+            border-color: var(--primary);
+        }
+    }


This and surrounding code (copied from InsightHistoryPanel.scss?) seems so specific, it feels messy to write the same thing in two files. Even if we're getting rid of InsightHistoryPanel eventually, I'd add some comment here to keep the scss files in sync... or then reuse the them in a more direct way.

Yep, I can leave a comment here. The better update is probably for me to consolidate all the dashboard items into the new mock design

mariusandra · 2021-08-25T12:13:42Z

frontend/src/scenes/saved-insights/savedInsightsLogic.ts

+            },
+        ],
+        searchTerm: [
+            '' as string,


nit: I think as string is not needed here

mariusandra · 2021-08-25T12:16:18Z

frontend/src/scenes/saved-insights/savedInsightsLogic.ts

+        },
+        setCreatedBy: () => {
+            actions.loadInsights()
+        },


It would indeed be great to persist these variables in the URL... and that's indeed easier to achieve if you have one filters reducer, versus multiple other reducers, though this can work as well.

liyiy · 2021-08-25T13:46:41Z

@mariusandra

Points 1-4 are very valid, the creation flow PR here should be tackling those issues and I will leave better instructions on how to reproduce my PRs 😄 .

Will incorporate 5, 6, 7, and 8

9 I will hold off for because I feel like that's something that can be improved upon in a follow up PR and I'd also have to update that functionality for dashboard item renaming too

liyiy · 2021-08-25T16:35:23Z

UX updates:

… into saved-insights-default-2

alexkim205 · 2021-08-25T21:44:24Z

Talked offline with @liyiy and decided to approve merge to master to avoid merge conflicts w/ #5702. I'll follow up with PR to address the above stuff.

liyiy added 2 commits August 17, 2021 09:09

set up card layout and insight search

59aaa55

add more filters

23a3256

timgl temporarily deployed to posthog-pr-5616 August 17, 2021 13:12 Inactive

liyiy added 3 commits August 17, 2021 13:38

clean up table

8219803

Merge branch 'master' into saved-insights-default-2

a7d3f6f

dropdown actions

22119a0

timgl temporarily deployed to posthog-pr-5616 August 19, 2021 04:02 Inactive

clean up

828ab97

timgl temporarily deployed to posthog-pr-5616 August 19, 2021 04:03 Inactive

fixes

946613c

timgl temporarily deployed to posthog-pr-5616 August 19, 2021 05:11 Inactive

date from filter

f50e55c

timgl temporarily deployed to posthog-pr-5616 August 19, 2021 06:28 Inactive

liyiy marked this pull request as ready for review August 19, 2021 06:29

proper date filtering

9f6ce5d

timgl temporarily deployed to posthog-pr-5616 August 19, 2021 18:52 Inactive

prettier

69b4d35

timgl temporarily deployed to posthog-pr-5616 August 19, 2021 19:01 Inactive

liyiy added 2 commits August 21, 2021 11:18

search_fields filters are not case insensitive

36e7ff7

various adjustments

2badb29

timgl temporarily deployed to posthog-pr-5616 August 22, 2021 00:14 Inactive

liyiy requested review from paolodamico, mariusandra and alexkim205 August 22, 2021 00:17

alexkim205 suggested changes Aug 24, 2021

View reviewed changes

Merge branch 'master' into saved-insights-default-2

30d79f6

clean up

9ea88e6

timgl temporarily deployed to posthog-pr-5616 August 25, 2021 04:21 Inactive

split nested ternary into if statements

1805567

timgl temporarily deployed to posthog-pr-5616 August 25, 2021 04:39 Inactive

Merge branch 'master' into saved-insights-default-2

84e4de5

mariusandra requested changes Aug 25, 2021

View reviewed changes

UX updates

38eb4a2

timgl temporarily deployed to posthog-pr-5616 August 25, 2021 16:33 Inactive

mariusandra added 2 commits August 25, 2021 20:30

add saved insights to storybook

de606ca

Merge branch 'saved-insights-default-2' of github.com:PostHog/posthog…

b28764f

… into saved-insights-default-2

mariusandra temporarily deployed to posthog-pr-5616 August 25, 2021 18:30 Inactive

alexkim205 self-assigned this Aug 25, 2021

alexkim205 self-requested a review August 25, 2021 21:44

alexkim205 approved these changes Aug 25, 2021

View reviewed changes

liyiy merged commit ce2f446 into master Aug 25, 2021

liyiy deleted the saved-insights-default-2 branch August 25, 2021 21:46

clarkus mentioned this pull request Sep 27, 2021

Updates to saved insights meta fields #6131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Saved insights default 2 #5616

Saved insights default 2 #5616

liyiy commented Aug 17, 2021 •

edited

Loading

liyiy commented Aug 17, 2021

clarkus commented Aug 17, 2021

clarkus commented Aug 19, 2021 •

edited

Loading

paolodamico commented Aug 24, 2021

alexkim205 Aug 24, 2021

liyiy Aug 25, 2021

clarkus Aug 25, 2021

alexkim205 Aug 24, 2021

mariusandra Aug 25, 2021

alexkim205 commented Aug 24, 2021

liyiy commented Aug 25, 2021

mariusandra left a comment •

edited

Loading

mariusandra Aug 25, 2021

liyiy Aug 25, 2021

mariusandra Aug 25, 2021

mariusandra Aug 25, 2021

liyiy commented Aug 25, 2021

liyiy commented Aug 25, 2021

alexkim205 commented Aug 25, 2021

Saved insights default 2 #5616

Saved insights default 2 #5616

Conversation

liyiy commented Aug 17, 2021 • edited Loading

Changes

Checklist

liyiy commented Aug 17, 2021

clarkus commented Aug 17, 2021

clarkus commented Aug 19, 2021 • edited Loading

paolodamico commented Aug 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexkim205 commented Aug 24, 2021

liyiy commented Aug 25, 2021

mariusandra left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyiy commented Aug 25, 2021

liyiy commented Aug 25, 2021

alexkim205 commented Aug 25, 2021

liyiy commented Aug 17, 2021 •

edited

Loading

clarkus commented Aug 19, 2021 •

edited

Loading

mariusandra left a comment •

edited

Loading