-
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): holdout groups UI #25772
Conversation
Size Change: +44 B (0%) Total Size: 1.15 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated8 snapshot changes in total. 0 added, 8 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
…osthog into experiment-holdouts-ui
…riment-holdouts-ui
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
…osthog into experiment-holdouts-ui
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
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.
haven't tried running locally yet, not sure if you've incorporated changes with my latest holdout_id
pr.
} | ||
size="xsmall" | ||
options={holdoutOptions} | ||
value={experiment.holdout || 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.
wouldn't this be the entire holdout
object?
@@ -52,8 +52,23 @@ export function VariantTag({ | |||
}): JSX.Element { | |||
const { experimentResults, getIndexForVariant } = useValues(experimentLogic({ experimentId })) | |||
|
|||
if (variantKey.startsWith('holdout-')) { |
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.
i guess we should disable user created variants starting with holdout-
🙈
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.
I think this check should be good enough for now: https://github.com/PostHog/posthog/pull/25772/files#diff-ac20870bd5fba7ae95bc9fde57e9108900e26f210382221cc1466059172748f3R55
Can add validation later if we run into issues
onChange={(rollout_percentage) => | ||
setHoldout({ | ||
filters: { | ||
groups: [{ properties: [], rollout_percentage }], |
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.
groups seem extra
…riment-holdouts-ui
Feedback addressed :) |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
Changes
1. List holdouts
2. Create/edit holdout
3. Edit holdout during experiment creation
4. Edit holdout in the Draft mode
5. Show holdout in the Launched mode (no longer editable)
6. Show results for the holdout
How did you test this code?
👀