Skip to content
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

chore: replace antd Select component in ObjectTags #20981

Merged
merged 21 commits into from
Mar 28, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ describe('the activity log logic', () => {
const actual = logic.values.humanizedActivity

expect(render(<>{actual[0].description}</>).container).toHaveTextContent(
'peter added tags 4 5 , and removed tags 2 3 on test insight'
'peter added tags 45, and removed tags 23 on test insight'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Believe this is just the new dom structure. Checked the stories and things still render nicely
Screenshot 2024-03-20 at 09 36 22

)
})

Expand Down
12 changes: 0 additions & 12 deletions frontend/src/lib/components/CloseButton.tsx

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ function DefinitionEdit(): JSX.Element {
<ObjectTags
className="definition-popover-edit-form-value"
tags={localDefinition.tags || []}
onChange={(_, tags) => setLocalDefinition({ tags })}
onChange={(tags) => setLocalDefinition({ tags })}
saving={false}
/>
</div>
Expand Down
161 changes: 58 additions & 103 deletions frontend/src/lib/components/ObjectTags/ObjectTags.tsx
Original file line number Diff line number Diff line change
@@ -1,18 +1,13 @@
// eslint-disable-next-line no-restricted-imports
import { CloseOutlined, SyncOutlined } from '@ant-design/icons'
import { IconPlus } from '@posthog/icons'
import { LemonTag, LemonTagType } from '@posthog/lemon-ui'
import { Select } from 'antd'
import { IconPencil, IconPlus } from '@posthog/icons'
import { LemonInputSelect, LemonTag, LemonTagType } from '@posthog/lemon-ui'
import clsx from 'clsx'
import { useActions, useValues } from 'kea'
import { objectTagsLogic } from 'lib/components/ObjectTags/objectTagsLogic'
import { Spinner } from 'lib/lemon-ui/Spinner/Spinner'
import { colorForString } from 'lib/utils'
import { CSSProperties, useMemo } from 'react'

import { AvailableFeature } from '~/types'

import { SelectGradientOverflow } from '../SelectGradientOverflow'
import { upgradeModalLogic } from '../UpgradeModal/upgradeModalLogic'

interface ObjectTagsPropsBase {
Expand All @@ -34,7 +29,7 @@ export type ObjectTagsProps =
| (ObjectTagsPropsBase & {
/** Tags CAN be added or removed.*/
staticOnly?: false
onChange?: (tag: string, tags?: string[], id?: string) => void
onChange?: (tags: string[]) => void
/** List of all tags that already exist. */
tagsAvailable?: string[] /** Whether this field should be gated behind a "paywall". */
})
Expand All @@ -55,15 +50,14 @@ export function ObjectTags({
tagsAvailable,
style = {},
staticOnly = false,
id, // For pages that allow multiple object tags
className,
'data-attr': dataAttr,
}: ObjectTagsProps): JSX.Element {
const objectTagId = useMemo(() => uniqueMemoizedIndex++, [])
const logic = objectTagsLogic({ id: objectTagId, onChange, tags })
const logic = objectTagsLogic({ id: objectTagId, onChange })
const { guardAvailableFeature } = useValues(upgradeModalLogic)
const { addingNewTag, cleanedNewTag, deletedTags } = useValues(logic)
const { setAddingNewTag, setNewTag, handleDelete, handleAdd } = useActions(logic)
const { editingTags } = useValues(logic)
const { setEditingTags, setTags } = useActions(logic)

/** Displaying nothing is confusing, so in case of empty static tags we use a dash as a placeholder */
const showPlaceholder = staticOnly && !tags?.length
Expand All @@ -77,99 +71,60 @@ export function ObjectTags({
})
}

const hasTags = tagsAvailable && tagsAvailable.length > 0

return (
// eslint-disable-next-line react/forbid-dom-props
<div style={style} className={clsx(className, 'flex flex-wrap gap-2 items-center')} data-attr={dataAttr}>
{showPlaceholder
? '—'
: tags
.filter((t) => !!t)
.map((tag, index) => {
return (
<LemonTag
key={index}
type={COLOR_OVERRIDES[tag] || colorForString(tag)}
style={{ marginRight: 0 }}
>
{tag}{' '}
{!staticOnly &&
onChange &&
(deletedTags.includes(tag) ? (
<SyncOutlined spin />
) : (
<CloseOutlined
className="click-outside-block"
style={{ cursor: 'pointer' }}
onClick={() =>
onGuardClick(() => {
handleDelete(tag)
})
}
/>
))}
</LemonTag>
)
})}
{saving && <Spinner />}
{!staticOnly && onChange && saving !== undefined && (
<span className="inline-flex font-normal">
<LemonTag
type="none"
onClick={() =>
onGuardClick(() => {
setAddingNewTag(true)
})
}
data-attr="button-add-tag"
icon={<IconPlus />}
className="border border-dashed"
style={{
display: addingNewTag ? 'none' : 'inline-flex',
}}
>
Add tag
</LemonTag>
{addingNewTag && (
<SelectGradientOverflow
size="small"
onBlur={() => setAddingNewTag(false)}
data-attr="new-tag-input"
autoFocus
allowClear
autoClearSearchValue
defaultOpen
showSearch
style={{ width: 160 }}
onChange={(changedValue) => handleAdd(changedValue)}
loading={saving}
onSearch={setNewTag}
placeholder='try "official"'
>
{cleanedNewTag ? (
<Select.Option
key={`${cleanedNewTag}_${id}`}
value={cleanedNewTag}
className="ph-no-capture"
data-attr="new-tag-option"
>
{cleanedNewTag}
</Select.Option>
) : (
(!tagsAvailable || !tagsAvailable.length) && (
<Select.Option key="__" value="__" disabled style={{ color: 'var(--muted)' }}>
Type to add a new tag
</Select.Option>
)
)}
{tagsAvailable &&
tagsAvailable.map((tag) => (
<Select.Option key={tag} value={tag} className="ph-no-capture">
{tag}
</Select.Option>
))}
</SelectGradientOverflow>
<div
// eslint-disable-next-line react/forbid-dom-props
style={style}
className={clsx(className, 'inline-flex flex-wrap space-x-1 items-center')}
data-attr={dataAttr}
>
{editingTags ? (
<LemonInputSelect
mode="multiple"
allowCustomValues
value={tags}
options={tagsAvailable?.map((t) => ({ key: t, label: t }))}
onChange={setTags}
onBlur={() => setEditingTags(false)}
loading={saving}
data-attr="new-tag-input"
placeholder='try "official"'
autoFocus
/>
) : (
<>
{showPlaceholder
? '—'
: tags
.filter((t) => !!t)
.map((tag, index) => {
return (
<LemonTag key={index} type={COLOR_OVERRIDES[tag] || colorForString(tag)}>
{tag}
</LemonTag>
)
})}
{!staticOnly && onChange && saving !== undefined && (
<span className="inline-flex font-normal">
<LemonTag
type="none"
onClick={() =>
onGuardClick(() => {
setEditingTags(true)
})
}
data-attr="button-add-tag"
icon={hasTags ? <IconPencil /> : <IconPlus />}
className="border border-dashed"
size="small"
>
{hasTags ? 'Edit tags' : 'Add tag'}
</LemonTag>
</span>
)}
</span>
</>
)}
</div>
)
Expand Down
52 changes: 11 additions & 41 deletions frontend/src/lib/components/ObjectTags/objectTagsLogic.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,68 +12,38 @@ describe('objectTagsLogic', () => {
props = {
id: 1,
onChange: jest.fn(),
tags: ['a', 'b', 'c'],
}
logic = objectTagsLogic(props)
logic.actions.setTags(['a', 'b', 'c'])
logic.mount()
})

describe('local tags state', () => {
it('initialization', async () => {
await expectLogic(logic).toMatchValues({
tags: ['a', 'b', 'c'],
addingNewTag: false,
newTag: '',
deletedTags: [],
editingTags: false,
})
})
it('handle adding a new tag', async () => {
it('cleans new tags', async () => {
await expectLogic(logic, async () => {
logic.actions.setNewTag('Nigh')
logic.actions.handleAdd('Nightly')
logic.actions.setEditingTags(true)
logic.actions.setTags(['a', 'b', 'c', 'Nightly'])
}).toMatchValues({
editingTags: true,
})
.toDispatchActions(['setNewTag'])
.toMatchValues({
newTag: 'Nigh',
cleanedNewTag: 'nigh', //user only needs to type part of the tag to find it in a list
})
.toDispatchActions(['handleAdd', logic.actionCreators.setTags(['a', 'b', 'c', 'nightly'])])
.toMatchValues({
tags: ['a', 'b', 'c', 'nightly'],
addingNewTag: false,
newTag: '',
})
// @ts-expect-error
const mockedOnChange = props.onChange?.mock
expect(mockedOnChange.calls.length).toBe(1)
expect(mockedOnChange.calls[0][0]).toBe('nightly')
expect(mockedOnChange.calls[0][1]).toEqual(['a', 'b', 'c', 'nightly'])
expect(mockedOnChange.calls[0][0]).toEqual(['a', 'b', 'c', 'nightly'])
})
it('noop on duplicate tag', async () => {
it('removes duplicate tags', async () => {
await expectLogic(logic, async () => {
logic.actions.handleAdd('a')
logic.actions.setTags(['a', 'nightly', 'b', 'c', 'nightly'])
})
.toDispatchActions(['handleAdd'])
.toNotHaveDispatchedActions(['setTags'])
.toMatchValues({
tags: ['a', 'b', 'c'],
})
// @ts-expect-error
expect(props.onChange?.mock.calls.length).toBe(0)
})
it('handle deleting a tag', async () => {
await expectLogic(logic, async () => {
logic.actions.handleDelete('a')
})
.toDispatchActions(['handleDelete', logic.actionCreators.setTags(['b', 'c'])])
.toMatchValues({
tags: ['b', 'c'],
})
// @ts-expect-error
const mockedOnChange = props.onChange?.mock
expect(mockedOnChange.calls.length).toBe(1)
expect(mockedOnChange.calls[0][0]).toBe('a')
expect(mockedOnChange.calls[0][1]).toEqual(['b', 'c'])
expect(mockedOnChange.calls[0][0]).toEqual(['a', 'nightly', 'b', 'c'])
})
})
})
Loading
Loading