Skip to content

Commit

Permalink
Enable persons modal on funnel trends (#5133)
Browse files Browse the repository at this point in the history 
* funnel trends persons call

* hide nonfunctioning funnel graph display options

* undo date year conversion

* avoid forced typing

* clean up and add comment

* use funnel_viz_type param for funnel graphs

* fix funnel_viz_type param router replacement bug

* fix types and potential display param bug

* funnel trends should not be an option for non-clickhouse

* reuse existing filters

* patch white screen death bug

* type predicate

* undo prettier
  • Loading branch information
@liyiy
liyiy committed Jul 20, 2021
1 parent 993278a commit 13cbf9f
Show file tree
Hide file tree
Showing 16 changed files with 148 additions and 110 deletions.
45 changes: 27 additions & 18 deletions frontend/src/lib/components/ChartFilter/ChartFilter.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,19 @@ import {
PieChartOutlined,
TableOutlined,
} from '@ant-design/icons'
import { ChartDisplayType, FilterType, ViewType } from '~/types'
import { ChartDisplayType, FilterType, FunnelVizType, ViewType } from '~/types'
import { preflightLogic } from 'scenes/PreflightCheck/logic'

interface ChartFilterProps {
filters: FilterType
onChange: (chartFilter: ChartDisplayType) => void
onChange: (chartFilter: ChartDisplayType | FunnelVizType) => void
disabled: boolean
}

export function ChartFilter({ filters, onChange, disabled }: ChartFilterProps): JSX.Element {
const { chartFilter } = useValues(chartFilterLogic)
const { setChartFilter } = useActions(chartFilterLogic)
const { preflight } = useValues(preflightLogic)

const linearDisabled = !!filters.session && filters.session === 'dist'
const cumulativeDisabled =
Expand Down Expand Up @@ -55,21 +57,28 @@ export function ChartFilter({ filters, onChange, disabled }: ChartFilterProps):

const options =
filters.insight === ViewType.FUNNELS
? [
{
value: ChartDisplayType.FunnelViz,
label: <Label icon={<OrderedListOutlined />}>Steps</Label>,
},
{
value: ChartDisplayType.ActionsLineGraphLinear,
label: (
<Label icon={<LineChartOutlined />}>
Trends
<WarningTag>BETA</WarningTag>
</Label>
),
},
]
? preflight?.is_clickhouse_enabled
? [
{
value: FunnelVizType.Steps,
label: <Label icon={<OrderedListOutlined />}>Steps</Label>,
},
{
value: FunnelVizType.Trends,
label: (
<Label icon={<LineChartOutlined />}>
Trends
<WarningTag>BETA</WarningTag>
</Label>
),
},
]
: [
{
value: FunnelVizType.Steps,
label: <Label icon={<OrderedListOutlined />}>Steps</Label>,
},
]
: [
{
label: 'Line Chart',
Expand Down Expand Up @@ -117,7 +126,7 @@ export function ChartFilter({ filters, onChange, disabled }: ChartFilterProps):
key="2"
defaultValue={filters.display || defaultDisplay}
value={chartFilter || defaultDisplay}
onChange={(value: ChartDisplayType) => {
onChange={(value: ChartDisplayType | FunnelVizType) => {
setChartFilter(value)
onChange(value)
}}
Expand Down
37 changes: 22 additions & 15 deletions frontend/src/lib/components/ChartFilter/chartFilterLogic.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
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)
}
},
}),
Expand Down
14 changes: 0 additions & 14 deletions frontend/src/scenes/dashboard/DashboardItem.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import { ActionsBarValueGraph } from 'scenes/trends/viz'

import relativeTime from 'dayjs/plugin/relativeTime'
import { eventUsageLogic } from 'lib/utils/eventUsageLogic'
import { FunnelHistogram } from 'scenes/funnels/FunnelHistogram'
import { featureFlagLogic } from 'lib/logic/featureFlagLogic'
import { FEATURE_FLAGS } from 'lib/constants'
import { Funnel } from 'scenes/funnels/Funnel'
Expand Down Expand Up @@ -138,19 +137,6 @@ export const displayMap: Record<DisplayedType, DisplayProps> = {
).url
},
},
FunnelsTimeToConvert: {
className: 'funnel-time-to-convert',
element: FunnelHistogram,
icon: BarChartOutlined,
viewText: 'View time conversion',
link: ({ id, dashboard, name, filters }: DashboardItemType): string => {
return combineUrl(
`/insights`,
{ insight: ViewType.FUNNELS, ...filters },
{ fromItem: id, fromItemName: name, fromDashboard: dashboard }
).url
},
},
RetentionContainer: {
className: 'retention',
element: RetentionContainer,
Expand Down
6 changes: 3 additions & 3 deletions frontend/src/scenes/funnels/Funnel.tsx
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useActions, useValues } from 'kea'
import React, { useEffect } from 'react'
import { ChartDisplayType, ChartParams } from '~/types'
import { ChartParams, FunnelVizType } from '~/types'
import { FunnelBarGraph } from './FunnelBarGraph'
import { FunnelHistogram } from './FunnelHistogram'
import { funnelLogic } from './funnelLogic'
Expand All @@ -14,9 +14,9 @@ export function Funnel(props: Omit<ChartParams, 'view'>): JSX.Element | null {
loadResults()
}, [])

const display = filters.display || props.filters.display
const funnel_viz_type = filters.funnel_viz_type || props.filters.funnel_viz_type

if (display == ChartDisplayType.FunnelsTimeToConvert) {
if (funnel_viz_type == FunnelVizType.TimeToConvert) {
return timeConversionBins?.bins?.length ? <FunnelHistogram {...props} /> : null
}

Expand Down
8 changes: 4 additions & 4 deletions frontend/src/scenes/funnels/FunnelCanvasLabel.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { insightLogic } from 'scenes/insights/insightLogic'
import { funnelLogic } from './funnelLogic'
import './FunnelCanvasLabel.scss'
import { chartFilterLogic } from 'lib/components/ChartFilter/chartFilterLogic'
import { ChartDisplayType } from '~/types'
import { FunnelVizType } from '~/types'

export function FunnelCanvasLabel(): JSX.Element | null {
const { stepsWithCount, histogramStep, conversionMetrics, clickhouseFeaturesEnabled } = useValues(funnelLogic)
Expand All @@ -21,7 +21,7 @@ export function FunnelCanvasLabel(): JSX.Element | null {

return (
<div className="funnel-canvas-label">
{allFilters.display === ChartDisplayType.FunnelViz && (
{allFilters.funnel_viz_type === FunnelVizType.Steps && (
<>
<span className="text-muted-alt">
<Tooltip title="Overall conversion rate for all users on the entire funnel.">
Expand All @@ -43,10 +43,10 @@ export function FunnelCanvasLabel(): JSX.Element | null {
</span>
<Button
type="link"
onClick={() => setChartFilter(FunnelVizType.TimeToConvert)}
disabled={
!clickhouseFeaturesEnabled || allFilters.display === ChartDisplayType.FunnelsTimeToConvert
!clickhouseFeaturesEnabled || allFilters.funnel_viz_type === FunnelVizType.TimeToConvert
}
onClick={() => setChartFilter(ChartDisplayType.FunnelsTimeToConvert)}
>
{humanFriendlyDuration(conversionMetrics.averageTime)}
</Button>
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/scenes/funnels/FunnelHistogram.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,17 @@ import { calcPercentage, getReferenceStep } from './funnelUtils'
import { funnelLogic } from './funnelLogic'
import { Histogram } from 'scenes/insights/Histogram'
import { insightLogic } from 'scenes/insights/insightLogic'
import { ChartDisplayType } from '~/types'

import './FunnelHistogram.scss'
import { FunnelVizType } from '~/types'
import { ChartParams } from '~/types'

export function FunnelHistogramHeader(): JSX.Element | null {
const { stepsWithCount, stepReference, histogramStepsDropdown } = useValues(funnelLogic)
const { changeHistogramStep } = useActions(funnelLogic)
const { allFilters } = useValues(insightLogic)

if (allFilters.display !== ChartDisplayType.FunnelsTimeToConvert) {
if (allFilters.funnel_viz_type !== FunnelVizType.TimeToConvert) {
return null
}

Expand Down
29 changes: 22 additions & 7 deletions frontend/src/scenes/funnels/FunnelViz.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ import FunnelGraph from 'funnel-graph-js'
import { Loading, humanFriendlyDuration } from 'lib/utils'
import { useActions, useValues, BindLogic } from 'kea'
import { funnelLogic } from './funnelLogic'
import { ACTIONS_LINE_GRAPH_LINEAR } from 'lib/constants'
import { LineGraph } from 'scenes/insights/LineGraph'
import { router } from 'kea-router'
import { InputNumber } from 'antd'
import { InputNumber, Row } from 'antd'
import { preflightLogic } from 'scenes/PreflightCheck/logic'
import { ChartParams } from '~/types'
import { ChartParams, FunnelVizType } from '~/types'
import { FunnelEmptyState } from 'scenes/insights/EmptyStates'

import './FunnelViz.scss'
import { personsModalLogic } from 'scenes/trends/personsModalLogic'

export function FunnelViz({
filters: defaultFilters,
Expand All @@ -32,6 +32,7 @@ export function FunnelViz({
areFiltersValid,
} = useValues(logic)
const { loadResults: loadFunnel, loadConversionWindow } = useActions(logic)
const { loadPeople } = useActions(personsModalLogic)
const {
hashParams: { fromItem },
} = useValues(router)
Expand All @@ -40,7 +41,7 @@ export function FunnelViz({
function buildChart(): void {
// Build and mount graph for default "flow" visualization.
// If steps are empty, new bargraph view is active, or linechart is visible, don't render flow graph.
if (!steps || steps.length === 0 || filters.display === ACTIONS_LINE_GRAPH_LINEAR) {
if (!steps || steps.length === 0 || filters.funnel_viz_type === FunnelVizType.Trends) {
return
}
if (container.current) {
Expand Down Expand Up @@ -103,10 +104,10 @@ export function FunnelViz({
)
}

if (filters.display === ACTIONS_LINE_GRAPH_LINEAR) {
if (filters.funnel_viz_type === FunnelVizType.Trends) {
return steps && steps.length > 0 && steps[0].labels ? (
<>
<div style={{ position: 'absolute', marginTop: -20, textAlign: 'center', width: '90%' }}>
<Row style={{ marginTop: -16, justifyContent: 'center' }}>
{preflight?.is_clickhouse_enabled && (
<>
converted within&nbsp;
Expand All @@ -121,7 +122,7 @@ export function FunnelViz({
</>
)}
% converted from first to last step
</div>
</Row>
<LineGraph
data-attr="trend-line-graph-funnel"
type="line"
Expand All @@ -132,6 +133,20 @@ export function FunnelViz({
dashboardItemId={dashboardItemId || fromItem}
inSharedMode={inSharedMode}
percentage={true}
onClick={
dashboardItemId
? null
: (point) => {
loadPeople({
action: { id: point.index, name: point.label, properties: [], type: 'actions' },
label: `Persons converted on ${point.label}`,
date_from: point.day,
date_to: point.day,
filters: filters,
saveOriginal: true,
})
}
}
/>
</>
) : null
Expand Down
21 changes: 13 additions & 8 deletions frontend/src/scenes/funnels/funnelLogic.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { funnelLogicType } from './funnelLogicType'
import {
EntityTypes,
FilterType,
ChartDisplayType,
FunnelVizType,
FunnelResult,
FunnelStep,
FunnelsTimeConversionBins,
Expand Down Expand Up @@ -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 } : {}),
...(filters.drop_off ? { drop_off: filters.drop_off } : {}),
interval: autocorrectInterval(filters),
breakdown: filters.breakdown || undefined,
breakdown_type: filters.breakdown_type || undefined,
Expand Down Expand Up @@ -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
Expand All @@ -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 } : {}),
})
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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)) {
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')],
}),
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/scenes/funnels/funnelUtils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ export function getSeriesPositionName(
}

export function humanizeStepCount(count: number): string {
return count > 9999 ? humanizeNumber(count, 2) : count.toLocaleString()
return count > 9999 ? humanizeNumber(count, 2) : count?.toLocaleString()
}

export function cleanBinResult(binsResult: FunnelsTimeConversionBins): FunnelsTimeConversionBins {
Expand Down
Loading

0 comments on commit 13cbf9f

Please sign in to comment.