-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(experiments): secondary metrics table #21164
Conversation
Size Change: 0 B Total Size: 824 kB ℹ️ View Unchanged
|
Needs more work.. |
…ndary-metrics-table
> | ||
{isModalReadOnly ? ( | ||
<div> | ||
<ExperimentInsightCreator insightProps={insightProps} /> |
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.
Here we need -> https://github.com/PostHog/posthog/blob/secondary-metrics-table/frontend/src/scenes/experiments/ExperimentResult.tsx#L215 the valid metric. Potentially worth refactoring ExperimentResults
component that you're using for the primary metric to make it work here
openModalToEditSecondaryMetric: ( | ||
metric: SecondaryExperimentMetric, | ||
metricIdx: number, | ||
readOnly: boolean = false |
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.
Instead of readOnly I think better to have a showResults
param, because its not read only version of the metric selector, but a completely different insight - to avoid confusion
@@ -17,3 +17,21 @@ export const transformResultFilters = (filters: Partial<FilterType>): Partial<Fi | |||
display: ChartDisplayType.ActionsLineGraphCumulative, | |||
}), | |||
}) | |||
|
|||
export function findKeyWithHighestNumber(obj: Record<string, number> | null): string | null { |
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.
Would be nice to consolidate all highest probability calculations into one
> | ||
{showResults && targetResults ? ( | ||
<div> | ||
<Query |
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.
consider extracting this into its own component in a follow up
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.
after we push the pairing changes, LGTM! :)
Changes
Notes:
How did you test this code?
👀