-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Enable persons modal on funnel trends #5133
Changes from 18 commits
c6c7379
01b5d25
4245d82
e258ea2
d9c9d60
e5a8dba
d146cae
484b941
9ea0ccc
6af3060
3100a86
9284c62
9b319f9
823d974
4bb60d7
5dbd0a1
b73a7bb
917853b
9aa4861
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,43 +2,50 @@ import { kea } from 'kea' | |
import { router } from 'kea-router' | ||
import { objectsEqual } from 'lib/utils' | ||
import { chartFilterLogicType } from './chartFilterLogicType' | ||
import { ChartDisplayType, ViewType } from '~/types' | ||
import { ChartDisplayType, FunnelVizType, ViewType } from '~/types' | ||
|
||
function isFunnelVizType(filter: FunnelVizType | ChartDisplayType): filter is FunnelVizType { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, previous comments about why we need |
||
return Object.values(FunnelVizType).includes(filter as FunnelVizType) | ||
} | ||
|
||
export const chartFilterLogic = kea<chartFilterLogicType>({ | ||
actions: () => ({ | ||
setChartFilter: (filter: ChartDisplayType) => ({ filter }), | ||
setChartFilter: (filter: ChartDisplayType | FunnelVizType) => ({ filter }), | ||
}), | ||
reducers: { | ||
chartFilter: [ | ||
null as null | ChartDisplayType, | ||
null as null | ChartDisplayType | FunnelVizType, | ||
{ | ||
setChartFilter: (_, { filter }) => filter, | ||
}, | ||
], | ||
}, | ||
listeners: ({ values }) => ({ | ||
setChartFilter: () => { | ||
const { display, ...searchParams } = router.values.searchParams // eslint-disable-line | ||
setChartFilter: ({ filter }) => { | ||
const { display, funnel_viz_type, ...searchParams } = router.values.searchParams // eslint-disable-line | ||
const { pathname } = router.values.location | ||
searchParams.display = values.chartFilter | ||
|
||
if (!objectsEqual(display, values.chartFilter)) { | ||
if (isFunnelVizType(filter)) { | ||
searchParams.funnel_viz_type = filter | ||
searchParams.display = ChartDisplayType.FunnelViz | ||
} else { | ||
searchParams.display = values.chartFilter | ||
} | ||
if ( | ||
(!isFunnelVizType(filter) && !objectsEqual(display, values.chartFilter)) || | ||
(isFunnelVizType(filter) && !objectsEqual(funnel_viz_type, values.chartFilter)) | ||
) { | ||
router.actions.replace(pathname, searchParams) | ||
} | ||
}, | ||
}), | ||
urlToAction: ({ actions }) => ({ | ||
'/insights': (_, { display, insight }) => { | ||
if (display) { | ||
'/insights': (_, { display, insight, funnel_viz_type }) => { | ||
if (display && !funnel_viz_type) { | ||
actions.setChartFilter(display) | ||
} else if (insight === ViewType.RETENTION) { | ||
actions.setChartFilter(ChartDisplayType.ActionsTable) | ||
} else if (insight === ViewType.FUNNELS) { | ||
if (display === ChartDisplayType.FunnelsTimeToConvert) { | ||
actions.setChartFilter(ChartDisplayType.FunnelsTimeToConvert) | ||
} else { | ||
actions.setChartFilter(ChartDisplayType.FunnelViz) | ||
} | ||
actions.setChartFilter(funnel_viz_type) | ||
} | ||
}, | ||
}), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ import { funnelLogicType } from './funnelLogicType' | |
import { | ||
EntityTypes, | ||
FilterType, | ||
ChartDisplayType, | ||
FunnelVizType, | ||
FunnelResult, | ||
FunnelStep, | ||
FunnelsTimeConversionBins, | ||
|
@@ -96,6 +96,8 @@ export const cleanFunnelParams = (filters: Partial<FilterType>): FilterType => { | |
...(filters.funnel_step ? { funnel_step: filters.funnel_step } : {}), | ||
...(filters.funnel_viz_type ? { funnel_viz_type: filters.funnel_viz_type } : {}), | ||
...(filters.funnel_step ? { funnel_to_step: filters.funnel_step } : {}), | ||
...(filters.entrance_period_start ? { entrance_period_start: filters.entrance_period_start } : {}), | ||
liyiy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
...(filters.drop_off ? { drop_off: filters.drop_off } : {}), | ||
interval: autocorrectInterval(filters), | ||
breakdown: filters.breakdown || undefined, | ||
breakdown_type: filters.breakdown_type || undefined, | ||
|
@@ -178,7 +180,7 @@ export const funnelLogic = kea<funnelLogicType>({ | |
} | ||
|
||
async function loadBinsResults(): Promise<FunnelsTimeConversionBins> { | ||
if (filters.display === ChartDisplayType.FunnelsTimeToConvert) { | ||
if (filters.funnel_viz_type === FunnelVizType.TimeToConvert) { | ||
try { | ||
// API specs (#5110) require neither funnel_{from|to}_step to be provided if querying | ||
// for all steps | ||
|
@@ -187,7 +189,6 @@ export const funnelLogic = kea<funnelLogicType>({ | |
const binsResult = await pollFunnel<FunnelsTimeConversionBins>({ | ||
...apiParams, | ||
...(refresh ? { refresh } : {}), | ||
funnel_viz_type: 'time_to_convert', | ||
...(!isAllSteps ? { funnel_from_step: histogramStep.from_step } : {}), | ||
...(!isAllSteps ? { funnel_to_step: histogramStep.to_step } : {}), | ||
}) | ||
|
@@ -307,8 +308,8 @@ export const funnelLogic = kea<funnelLogicType>({ | |
], | ||
showBarGraph: [ | ||
() => [selectors.filters], | ||
({ display }: { display: ChartDisplayType }) => | ||
display === ChartDisplayType.FunnelViz || display === ChartDisplayType.FunnelsTimeToConvert, | ||
({ funnel_viz_type }: { funnel_viz_type: FunnelVizType }) => | ||
funnel_viz_type === FunnelVizType.Steps || funnel_viz_type === FunnelVizType.TimeToConvert, | ||
], | ||
clickhouseFeaturesEnabled: [ | ||
() => [featureFlagLogic.selectors.featureFlags, selectors.preflight], | ||
|
@@ -426,10 +427,14 @@ export const funnelLogic = kea<funnelLogicType>({ | |
], | ||
steps: [ | ||
() => [selectors.results, selectors.stepsWithNestedBreakdown, selectors.filters], | ||
(results, stepsWithNestedBreakdown, filters): FunnelStepWithNestedBreakdown[] => | ||
!!filters.breakdown | ||
(results, stepsWithNestedBreakdown, filters): FunnelStepWithNestedBreakdown[] => { | ||
if (!Array.isArray(results)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 😩 |
||
return [] | ||
} | ||
return !!filters.breakdown | ||
? stepsWithNestedBreakdown | ||
: ([...results] as FunnelStep[]).sort((a, b) => a.order - b.order), | ||
: ([...results] as FunnelStep[]).sort((a, b) => a.order - b.order) | ||
}, | ||
], | ||
stepsWithCount: [() => [selectors.steps], (steps) => steps.filter((step) => typeof step.count === 'number')], | ||
}), | ||
|
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.
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 withFunnelVizType
, OR have e.g.ChartDisplayType.FUNNELS
cover everything and haveFunnelVizType
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
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.
Scratch that, I really think
FunnelVizType
should be removed. We already haveViewType.FUNNELS
to describe funnels overall.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.
that's where the typing will come from for
funnel_viz_type
, see this comment: #5133 (comment)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.
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.
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.
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".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.
I think it's also a little confusing to reuse
ChartDisplayType
for something like time conversion because that technically uses theActionsLineGraphLinear
display type and would also probably cause confusion on the frontend if we checked forinsights === FUNNELS && display === ChartDisplayType.ActionsLineGraphLinear
vs justfunnel_viz_type === FunnelVizType.Trends/TimeConversion/whatever
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.
but also agree we should make our chart/display type system easier to navigate in general :o