Enable persons modal on funnel trends

[liyiy]

Changes

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

The API does allow for drop-off users to return but after talking with @clarkus, we decided to leave that as an option for a future iteration, since the funnel trends graph currently only visualizes conversions, aka people that didn't drop off.

Screen.Recording.2021-07-14.at.3.48.40.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)

[Twixes]

I think it's okay, though not ideal, to only show people who converted – but definitely confusing for me if that's not clarified in the modal.

[mariusandra]

Hey, I guess some of the comments here are still worth implementing

Plus I see this now with postgres:
https://github.com/PostHog/posthog/pull/5133/files/d146cae005a42ffb916b67fbd721e51869077851#r670820614

(not sure why I have the old tooltips...)

[mariusandra]

Looks good to me. There's one type-narrowing-related comment I still managed to find during my last run at the code. Fine to improve later as well.

[mariusandra]

Oh, and enjoy the merge conflicts :D

[liyiy]

This was causing a white screen of death for funnel time conversion because it was trying to spread an object of time conversion bin result values. This is a temporary-ish fix, I will take a deeper look in a different PR. Not this one... no more merge conflicts plz 😩

[samwinslow]

I think the addition of this type just confuses things, especially if they're going to be used interchangeably. My preferred approach would be to put everything in ChartDisplayType and do away with FunnelVizType, OR have e.g. ChartDisplayType.FUNNELS cover everything and have FunnelVizType make it more specific.

What do you think @mariusandra @alexkim205 ? I don't have a strong opinion here but feel like we should agree on a convention

[samwinslow]

Scratch that, I really think FunnelVizType should be removed. We already have ViewType.FUNNELS to describe funnels overall.

[liyiy]

that's where the typing will come from for funnel_viz_type, see this comment: #5133 (comment)

[neilkakkar]

To clarify, FunnelVizType is the type of funnel visualization (since that's separate from overarching visualizations (like Trends or Funnels) )

Although, valid question whether ChartFilterProps, the top level interface should expect top level visualisation type or specific-to-funnels visualization type as well.

[samwinslow]

Right, @neilkakkar , ViewType includes Trends, Stickiness, Funnels, Retention, etc. and corresponds to overall type of query.

Then ChartDisplayType describes, for instance, linear line graph, cumulative line graph, bar chart, pie, etc. and seems like the logical place to distinguish between time to convert, steps, and historical trends as those are "types of visualization".

[liyiy]

I think it's also a little confusing to reuse ChartDisplayType for something like time conversion because that technically uses the ActionsLineGraphLinear display type and would also probably cause confusion on the frontend if we checked for insights === FUNNELS && display === ChartDisplayType.ActionsLineGraphLinear vs just funnel_viz_type === FunnelVizType.Trends/TimeConversion/whatever

[liyiy]

but also agree we should make our chart/display type system easier to navigate in general :o

[samwinslow]

Again, previous comments about why we need ViewType and FunnelVizType and ChartDisplayType apply here.

[samwinslow]

LGTM I think we could just benefit from some clarity as to why ChartDisplayType can't do what we want FunnelVizType to do, because ViewType already has a catch-all for funnels.

[liyiy]

@samwinslow

I agree adding another "what graph type is it" param is confusing, ideally we could've just kept the model of "display=FunnelsTimeToConvert" and updated the backend to check that instead of funnel_viz_type. But not 100% sure if that's ideal for the API?

I think the intention of funnel_viz_type is to add slightly more clarity to the relationship between the funnels parent type and its trends/steps/timetoconvert children types.

I can, however, create a new action that handles the funnel_viz_type param setting logic specifically, and maybe that'll improve things, but in another PR plz 😄

[mariusandra]

Would something like work?

type TrendsDisplayType = ChartDisplayType // rename it
type ChartFilter = TrendsDisplayType | FunnelVizType

props = {
    onChange: (chartFilter: ChartFilter) => void
}

The idea behind keeping the two display types separate is that they're separate trends views with different dropdowns. The ChartDisplayType needs anyway some cleanup, and this seemed slightly outside the scope here.