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(settings): Clarify persons on events setting #22953

Merged
merged 30 commits into from
Jun 20, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
871256a
chore(settings): Clarify persons on events setting
Twixes Jun 7, 2024
40ec23b
Update UI snapshots for `chromium` (1)
github-actions[bot] Jun 13, 2024
9780550
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 13, 2024
dee2874
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 13, 2024
e5af593
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 13, 2024
b5e80fa
Merge branch 'master' into poe-setting-clearer
Twixes Jun 13, 2024
8617413
Add docs link
Twixes Jun 14, 2024
eef4b2f
Merge branch 'master' into poe-setting-clearer
Twixes Jun 14, 2024
384b3ef
Update UI snapshots for `chromium` (1)
github-actions[bot] Jun 14, 2024
6c0b6be
Update UI snapshots for `chromium` (1)
github-actions[bot] Jun 14, 2024
8bc4a8b
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 14, 2024
6591052
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 14, 2024
eb1e5d5
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 14, 2024
55c3d04
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 14, 2024
8c23a39
Update UI snapshots for `chromium` (1)
github-actions[bot] Jun 14, 2024
de03754
Update UI snapshots for `chromium` (1)
github-actions[bot] Jun 14, 2024
0a6352c
Say "Recommended"
Twixes Jun 19, 2024
744549b
Update UI snapshots for `chromium` (1)
github-actions[bot] Jun 19, 2024
e372770
Update UI snapshots for `chromium` (1)
github-actions[bot] Jun 19, 2024
246ac0c
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 19, 2024
a298e9f
Update UI snapshots for `chromium` (1)
github-actions[bot] Jun 19, 2024
3de619b
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 19, 2024
503edc3
Update UI snapshots for `chromium` (1)
github-actions[bot] Jun 19, 2024
3d561e1
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 19, 2024
60e10a4
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 19, 2024
b8661e7
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 19, 2024
74b89ec
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 19, 2024
8a09619
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 19, 2024
6978e41
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 19, 2024
1d2563c
Update UI snapshots for `chromium` (2)
github-actions[bot] Jun 19, 2024
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
Binary file modified frontend/__snapshots__/lemon-ui-lemon-radio--default--dark.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified frontend/__snapshots__/lemon-ui-lemon-radio--default--light.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file not shown.
Binary file not shown.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
16 changes: 11 additions & 5 deletions frontend/src/lib/lemon-ui/LemonRadio/LemonRadio.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const meta: Meta<typeof LemonRadio> = {
options: [
{ value: 'calendar', label: 'Calendar' },
{ value: 'calculator', label: 'Calculator' },
{ value: 'banana', label: 'Banana' },
{ value: 'banana', label: 'Banana', disabledReason: 'Bananas are not allowed on pizza' },
{ value: 'settings', label: 'Settings' },
] as LemonRadioOption<string>[],
},
Expand All @@ -38,11 +38,17 @@ const Template: StoryFn<typeof LemonRadio> = (props: Omit<LemonRadioProps<any>,
export const Default: Story = Template.bind({})
Default.args = {}

export const Disabled: Story = Template.bind({})
Disabled.args = {
export const WithDescriptions: Story = Template.bind({})
WithDescriptions.args = {
options: [
{ value: 'calendar', label: 'Calendar' },
{ value: 'calculator', label: 'Calculator' },
{ value: 'banana', label: 'Banana', disabledReason: 'Bananas are not allowed' },
{ value: 'calculator', label: 'Calculator', description: '2.1 + 2.01 = 4.109999999999999' },
{
value: 'banana',
label: 'Banana',
disabledReason: 'Bananas are not allowed on pizza',
description:
'Note: The banana addon ships from Costa Rica, which will add 2 working days of a delay to your order.',
},
],
}
8 changes: 6 additions & 2 deletions frontend/src/lib/lemon-ui/LemonRadio/LemonRadio.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Tooltip } from 'lib/lemon-ui/Tooltip'

export interface LemonRadioOption<T extends React.Key> {
label: string | JSX.Element
description?: string | JSX.Element
value: T
disabledReason?: string
'data-attr'?: string
Expand All @@ -25,12 +26,12 @@ export function LemonRadio<T extends React.Key>({
}: LemonRadioProps<T>): JSX.Element {
return (
<div className={clsx('flex flex-col gap-2 font-medium', className)}>
{options.map(({ value, label, disabledReason, ...optionProps }) => {
{options.map(({ value, label, disabledReason, description, ...optionProps }) => {
const content = (
<label
key={value}
className={clsx(
'flex items-center space-x-2',
'grid items-center gap-x-2 grid-cols-[min-content_auto] text-sm',
disabledReason ? 'text-muted cursor-not-allowed' : 'cursor-pointer'
)}
>
Expand All @@ -48,6 +49,9 @@ export function LemonRadio<T extends React.Key>({
{...optionProps}
/>
<span>{label}</span>
{description && (
<div className="text-muted row-start-2 col-start-2 text-pretty">{description}</div>
)}
</label>
)

Expand Down
46 changes: 23 additions & 23 deletions frontend/src/scenes/settings/project/PersonsOnEvents.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { LemonTag } from '@posthog/lemon-ui'
import { useActions, useValues } from 'kea'
import { LemonButton } from 'lib/lemon-ui/LemonButton'
import { LemonRadio, LemonRadioOption } from 'lib/lemon-ui/LemonRadio'
Expand All @@ -9,39 +10,38 @@ import { HogQLQueryModifiers } from '~/queries/schema'

type PoEMode = NonNullable<HogQLQueryModifiers['personsOnEventsMode']>

const poeOptions: LemonRadioOption<PoEMode>[] = [
const POE_OPTIONS: LemonRadioOption<PoEMode>[] = [
{
value: 'person_id_no_override_properties_on_events',
value: 'person_id_override_properties_on_events',
label: (
<span className="inline-flex items-center gap-1.5">
Use person properties from the time of the event<LemonTag>DEFAULT</LemonTag>
</span>
),
description: (
<>
<div>Use person ids and properties from the time of the event.</div>
<div className="text-muted">
May show higher unique user counts due to not using latest person ids. You probably want one of the
other options. Fastest queries.
</div>
Fast queries. If the person property is updated, query results on past data <em>won't</em> change.
</>
),
},
{
value: 'person_id_override_properties_on_events',
label: (
value: 'person_id_override_properties_joined',
label: 'Use person properties as of running the query',
description: (
<>
<div>Use person properties from the time of the event.</div>
<div className="text-muted">
Fast queries. If person property is updated, then query results on past data won't change.
</div>
Slower queries. If the person property is updated, query results on past data <em>will</em> change
accordingly.
</>
),
},
{
value: 'person_id_override_properties_joined',
label: (
value: 'person_id_no_override_properties_on_events',
label: 'Use person IDs and person properties from the time of the event',
description: (
<>
<div>Use latest person properties.</div>
<div className="text-muted">
Slower queries. If person property is updated, then query results on past data will change to
reflect it.
</div>
Fastest queries,{' '}
<span className="underline">but funnels and unique user counts will be inaccurate</span>. If the person
property is updated, query results on past data <em>won't</em> change.
</>
),
},
Expand All @@ -51,7 +51,7 @@ export function PersonsOnEvents(): JSX.Element {
const { updateCurrentTeam } = useActions(teamLogic)
const { reportPoEModeUpdated } = useActions(eventUsageLogic)
const { currentTeam } = useValues(teamLogic)
const savedPoEMode =
const savedPoEMode: PoEMode =
currentTeam?.modifiers?.personsOnEventsMode ?? currentTeam?.default_modifiers?.personsOnEventsMode ?? 'disabled'
const [poeMode, setPoeMode] = useState<PoEMode>(savedPoEMode)

Expand All @@ -62,8 +62,8 @@ export function PersonsOnEvents(): JSX.Element {

return (
<>
<p>Choose how to query your event data with person filters.</p>
<LemonRadio value={poeMode} onChange={setPoeMode} options={poeOptions} />
<p>Choose the behavior of person property filters.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>Choose the behavior of person property filters.</p>
<p>Choose how properties relating to a person change in comparison to the events setting them.</p>

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like this is a very complex sentence 😅 but it is a complex setting. Actually, I just recalled Tiina made a docs page about this. Added a link to it for those interested in the specifics of "behavior" here:
Screenshot 2024-06-14 at 11 01 44

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my main point here is that this doesn't only affect filters, but everything related to person properties. So maybe just "Choose the behavior of person properties."?

Docs link is great 👍

Copy link
Member Author

@Twixes Twixes Jun 14, 2024

Choose a reason for hiding this comment

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

Hmm, is there something other than filters affected? (and person IDs for the "fastest" option) So, what I'm thinking is that it's actually a useful distinction to clarify whether this setting is query-time or ingestion-time, and it's the former – so saying it's about the "filters" might be useful in that sense, because it's clear the data stays the same, it's just a different way of querying it

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is there something other than filters affected?

Breakdowns, aggregations, etc.

So, what I'm thinking is that it's actually a useful distinction to clarify whether this setting is query-time or ingestion-time,

Yup, super useful! We could also adapt the header to say "Person properties querying" instead of the current "Person properties mode".

so saying it's about the "filters" might be useful in that sense, because it's clear the data stays the same, it's just a different way of querying it

In any case happy for this to go in.

<LemonRadio value={poeMode} onChange={setPoeMode} options={POE_OPTIONS} />
<div className="mt-4">
<LemonButton
type="primary"
Expand Down
Loading