Refactor old cohort event selector (#8416)

* create even simpler TaxonomicStringPopup * remove functional programming bugs * full width by default * findActionName helper * use taxonomic filter for choosing cohort event/action
PostHog · Feb 3, 2022 · 28a16fe · 28a16fe
1 parent c3b23d7
commit 28a16fe
Show file tree

Hide file tree

Showing 8 changed files with 69 additions and 144 deletions.
diff --git a/frontend/src/lib/components/TaxonomicPopup/TaxonomicPopup.tsx b/frontend/src/lib/components/TaxonomicPopup/TaxonomicPopup.tsx
@@ -1,22 +1,36 @@
 import './TaxonomicPopup.scss'
 import { Popup } from 'lib/components/Popup/Popup'
 import { TaxonomicFilter } from 'lib/components/TaxonomicFilter/TaxonomicFilter'
-import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types'
+import { TaxonomicFilterGroupType, TaxonomicFilterValue } from 'lib/components/TaxonomicFilter/types'
 import React, { useState } from 'react'
 import { Button } from 'antd'
 import { DownOutlined } from '@ant-design/icons'
+import clsx from 'clsx'
 
-export interface TaxonomicPopupProps {
+export interface TaxonomicPopupProps<ValueType = TaxonomicFilterValue> {
     groupType: TaxonomicFilterGroupType
-    value?: string
-    onChange: (value: string, groupType: TaxonomicFilterGroupType) => void
+    value?: ValueType
+    onChange: (value: ValueType, groupType: TaxonomicFilterGroupType) => void
 
     groupTypes?: TaxonomicFilterGroupType[]
-    renderValue?: (value: string) => JSX.Element
+    renderValue?: (value: ValueType) => JSX.Element
     dataAttr?: string
     eventNames?: string[]
     placeholder?: React.ReactNode
     style?: React.CSSProperties
+    fullWidth?: boolean
+}
+
+/** Like TaxonomicPopup, but convenient when you know you will only use string values */
+export function TaxonomicStringPopup(props: TaxonomicPopupProps<string>): JSX.Element {
+    return (
+        <TaxonomicPopup
+            {...props}
+            value={String(props.value)}
+            onChange={(value, groupType) => props.onChange?.(String(value), groupType)}
+            renderValue={(value) => props.renderValue?.(String(value)) ?? <>{String(props.value)}</>}
+        />
+    )
 }
 
 export function TaxonomicPopup({
@@ -29,6 +43,7 @@ export function TaxonomicPopup({
     eventNames = [],
     placeholder = 'Please select',
     style,
+    fullWidth = true,
 }: TaxonomicPopupProps): JSX.Element {
     const [visible, setVisible] = useState(false)
 
@@ -39,7 +54,7 @@ export function TaxonomicPopup({
                     groupType={groupType}
                     value={value}
                     onChange={({ type }, payload) => {
-                        onChange?.(String(payload), type)
+                        onChange?.(payload, type)
                         setVisible(false)
                     }}
                     taxonomicGroupTypes={groupTypes ?? [groupType]}
@@ -54,7 +69,7 @@ export function TaxonomicPopup({
                     data-attr={dataAttr}
                     onClick={() => setVisible(!visible)}
                     ref={setRef}
-                    className="TaxonomicPopup__button"
+                    className={clsx('TaxonomicPopup__button', { 'full-width': fullWidth })}
                     style={style}
                 >
                     <span className="text-overflow" style={{ maxWidth: '100%' }}>

diff --git a/frontend/src/lib/components/TaxonomicPopup/__stories__/TaxonomicPopup.stories.tsx b/frontend/src/lib/components/TaxonomicPopup/__stories__/TaxonomicPopup.stories.tsx
@@ -1,5 +1,5 @@
 import React, { useState } from 'react'
-import { TaxonomicPopup } from '../TaxonomicPopup'
+import { TaxonomicPopup, TaxonomicStringPopup } from '../TaxonomicPopup'
 import { personPropertiesModel } from '~/models/personPropertiesModel'
 import { cohortsModel } from '~/models/cohortsModel'
 import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types'
@@ -11,7 +11,7 @@ export default {
     title: 'PostHog/Components/TaxonomicPopup',
 }
 
-export const OneCategory = (): JSX.Element => {
+export const TaxonomicStringPopupOneCategory = (): JSX.Element => {
     const [value, setValue] = useState<string | undefined>('$browser')
 
     return (
@@ -22,7 +22,7 @@ export const OneCategory = (): JSX.Element => {
                 defaultFilterMocks()
             }}
         >
-            <TaxonomicPopup
+            <TaxonomicStringPopup
                 groupType={TaxonomicFilterGroupType.PersonProperties}
                 value={value}
                 onChange={setValue}
@@ -33,7 +33,7 @@ export const OneCategory = (): JSX.Element => {
 }
 
 export const MultipleCategories = (): JSX.Element => {
-    const [value, setValue] = useState<string | undefined>(undefined)
+    const [value, setValue] = useState<string | number | undefined>(undefined)
     const [group, setGroup] = useState(TaxonomicFilterGroupType.PersonProperties)
 
     return (

diff --git a/frontend/src/models/actionsModel.ts b/frontend/src/models/actionsModel.ts
@@ -7,6 +7,10 @@ interface ActionsModelProps {
     params?: string
 }
 
+export function findActionName(id: number): string | null {
+    return actionsModel.findMounted()?.values.actions.find((a) => a.id === id)?.name || null
+}
+
 export const actionsModel = kea<actionsModelType<ActionsModelProps>>({
     path: ['models', 'actionsModel'],
     props: {} as ActionsModelProps,

diff --git a/frontend/src/scenes/actions/EventName.tsx b/frontend/src/scenes/actions/EventName.tsx
@@ -1,6 +1,6 @@
 import React from 'react'
 import { PropertyKeyInfo } from 'lib/components/PropertyKeyInfo'
-import { TaxonomicPopup } from 'lib/components/TaxonomicPopup/TaxonomicPopup'
+import { TaxonomicStringPopup } from 'lib/components/TaxonomicPopup/TaxonomicPopup'
 import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types'
 
 interface EventNameInterface {
@@ -10,11 +10,11 @@ interface EventNameInterface {
 
 export function EventName({ value, onChange }: EventNameInterface): JSX.Element {
     return (
-        <TaxonomicPopup
+        <TaxonomicStringPopup
             groupType={TaxonomicFilterGroupType.Events}
             onChange={onChange}
             value={value}
-            style={{ width: '100%', maxWidth: '24rem' }}
+            style={{ maxWidth: '24rem' }}
             placeholder="Choose an event"
             dataAttr="event-name-box"
             renderValue={(v) => <PropertyKeyInfo value={v} />}

diff --git a/frontend/src/scenes/cohorts/CohortMatchingCriteriaSection/CohortEntityFilterBox.tsx b/frontend/src/scenes/cohorts/CohortMatchingCriteriaSection/CohortEntityFilterBox.tsx
diff --git a/frontend/src/scenes/cohorts/CohortMatchingCriteriaSection/CohortMatchingCriteriaSection.tsx b/frontend/src/scenes/cohorts/CohortMatchingCriteriaSection/CohortMatchingCriteriaSection.tsx
@@ -11,20 +11,20 @@ export function CohortMatchingCriteriaSection({ logic }: { logic: BuiltLogic<coh
     const { setCohort, onCriteriaChange } = useActions(logic)
     const { cohort, submitted } = useValues(logic)
     const onAddGroup = (): void => {
-        cohort.groups = [
-            ...cohort.groups,
-            {
-                id: Math.random().toString().substr(2, 5),
-                matchType: PROPERTY_MATCH_TYPE,
-                properties: [],
-            },
-        ]
-        setCohort({ ...cohort })
+        setCohort({
+            ...cohort,
+            groups: [
+                ...cohort.groups,
+                {
+                    id: Math.random().toString().substr(2, 5),
+                    matchType: PROPERTY_MATCH_TYPE,
+                    properties: [],
+                },
+            ],
+        })
     }
-
     const onRemoveGroup = (index: number): void => {
-        cohort.groups.splice(index, 1)
-        setCohort({ ...cohort })
+        setCohort({ ...cohort, groups: cohort.groups.filter((_, i) => i !== index) })
     }
     const addButton = (
         <div style={{ marginTop: 8, marginBottom: 8 }}>

diff --git a/frontend/src/scenes/cohorts/CohortMatchingCriteriaSection/MatchCriteriaSelector.tsx b/frontend/src/scenes/cohorts/CohortMatchingCriteriaSection/MatchCriteriaSelector.tsx
@@ -1,13 +1,13 @@
-import React, { useState } from 'react'
-import { Button, Col, Input, Row, Select } from 'antd'
-import { CohortEntityFilterBox } from './CohortEntityFilterBox'
+import React from 'react'
+import { Col, Input, Row, Select } from 'antd'
 import { PropertyKeyInfo } from 'lib/components/PropertyKeyInfo'
-import { SelectDownIcon } from 'lib/components/SelectDownIcon'
 import { CohortGroupType, MatchType } from '~/types'
-import { ACTION_TYPE, ENTITY_MATCH_TYPE, EVENT_TYPE, PROPERTY_MATCH_TYPE } from 'lib/constants'
+import { ENTITY_MATCH_TYPE, PROPERTY_MATCH_TYPE } from 'lib/constants'
 import { PropertyFilters } from 'lib/components/PropertyFilters/PropertyFilters'
 import { DeleteOutlined } from '@ant-design/icons'
 import { TaxonomicFilterGroupType } from 'lib/components/TaxonomicFilter/types'
+import { TaxonomicPopup } from 'lib/components/TaxonomicPopup/TaxonomicPopup'
+import { findActionName } from '~/models/actionsModel'
 
 const { Option } = Select
 
@@ -128,8 +128,6 @@ function EntityCriteriaRow({
     onEntityCriteriaChange: (group: Partial<CohortGroupType>) => void
     group: CohortGroupType
 }): JSX.Element {
-    const [open, setOpen] = useState(false)
-
     const { label, days, count_operator, count } = group
 
     const onOperatorChange = (newCountOperator: string): void => {
@@ -144,33 +142,30 @@ function EntityCriteriaRow({
         onEntityCriteriaChange({ count: newCount })
     }
 
-    const onEntityChange = (type: any, id: string | number, newLabel: string): void => {
-        if (type === EVENT_TYPE && typeof id === 'string') {
-            onEntityCriteriaChange({ event_id: id, label: newLabel })
-        } else if (type === ACTION_TYPE && typeof id === 'number') {
-            onEntityCriteriaChange({ action_id: id, label: newLabel })
+    const onEntityChange = (type: TaxonomicFilterGroupType, id: string | number): void => {
+        if (type === TaxonomicFilterGroupType.Events && typeof id === 'string') {
+            onEntityCriteriaChange({ event_id: id, action_id: undefined, label: id })
+        } else if (type === TaxonomicFilterGroupType.Actions && typeof id === 'number') {
+            onEntityCriteriaChange({
+                action_id: id,
+                event_id: undefined,
+                label: findActionName(id) || `Action #${id}`,
+            })
         }
-        setOpen(false)
     }
 
     return (
         <div style={{ marginTop: 16, width: '100%' }}>
             <Row gutter={8}>
                 <Col flex="auto">
-                    <Button
-                        onClick={() => setOpen(!open)}
-                        className="full-width"
-                        style={{
-                            display: 'flex',
-                            justifyContent: 'space-between',
-                            alignItems: 'center',
-                        }}
-                        data-attr="edit-cohort-entity-filter"
-                    >
-                        <PropertyKeyInfo value={label || 'Select an event'} />
-                        <SelectDownIcon className="text-muted" />
-                    </Button>
-                    <CohortEntityFilterBox open={open} onSelect={onEntityChange} />
+                    <TaxonomicPopup
+                        groupTypes={[TaxonomicFilterGroupType.Events, TaxonomicFilterGroupType.Actions]}
+                        groupType={group.action_id ? TaxonomicFilterGroupType.Actions : TaxonomicFilterGroupType.Events}
+                        value={group.action_id || group.event_id}
+                        onChange={(value, groupType) => onEntityChange(groupType, value)}
+                        renderValue={() => <PropertyKeyInfo value={label || 'Select an event'} disablePopover={true} />}
+                        dataAttr="edit-cohort-entity-filter"
+                    />
                 </Col>
                 <Col span={4}>
                     <OperatorSelect value={count_operator} onChange={onOperatorChange} />

diff --git a/frontend/src/scenes/insights/ActionFilter/ActionFilterRow/ActionFilterRow.tsx b/frontend/src/scenes/insights/ActionFilter/ActionFilterRow/ActionFilterRow.tsx
@@ -32,7 +32,7 @@ import { apiValueToMathType, mathsLogic, mathTypeToApiValues } from 'scenes/tren
 import { GroupsIntroductionOption } from 'lib/introductions/GroupsIntroductionOption'
 import { actionsModel } from '~/models/actionsModel'
 import { PropertyKeyInfo } from 'lib/components/PropertyKeyInfo'
-import { TaxonomicPopup } from 'lib/components/TaxonomicPopup/TaxonomicPopup'
+import { TaxonomicStringPopup } from 'lib/components/TaxonomicPopup/TaxonomicPopup'
 
 const determineFilterLabel = (visible: boolean, filter: Partial<ActionFilter>): string => {
     if (visible) {
@@ -373,7 +373,7 @@ export function ActionFilterRow({
                                                 maxWidth: `calc(50% - 16px${showSeriesIndicator ? ' - 32px' : ''})`,
                                             }}
                                         >
-                                            <TaxonomicPopup
+                                            <TaxonomicStringPopup
                                                 groupType={TaxonomicFilterGroupType.NumericalEventProperties}
                                                 value={mathProperty}
                                                 onChange={(currentValue) => onMathPropertySelect(index, currentValue)}