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

fix(experiments): add exposure column when variants > 4 #20140

Merged
merged 6 commits into from
Feb 6, 2024

Conversation

jurajmajerik
Copy link
Contributor

Changes

Add the exposure column for when the variants count > 4

image

How did you test this code?

👀

Copy link
Contributor

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

very nice :)

Kinda sus no snapshots update, but I assume this is because we're missing snapshots for > 4 variants case, bonus points if you add it 🙈

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Size Change: 0 B

Total Size: 2.22 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 2.22 MB

compressed-size-action

@@ -67,6 +67,14 @@ export function ExperimentResult(): JSX.Element {
</Col>
))}
</div>
<div className="flex justify-between py-2 border-t">
Copy link
Contributor

Choose a reason for hiding this comment

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

small nit for Line 60: Can you update it to say metric when appropriate as well, like here: https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/experiments/ExperimentResult.tsx#L107

Copy link
Contributor Author

@jurajmajerik jurajmajerik Feb 6, 2024

Choose a reason for hiding this comment

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

eh, I'm not sure I understand, can you double check? 👇 (assuming you meant line 70)

b016655#diff-ed1b2dce372049018efc84c25054c92c9b1f2b8f4b374314e2475fae436f67a9R71-R73

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for not being clear, I meant line 60. Just calling it Exposure here is fine, but when using experiment goals like 'avg per user', calling the number as "count" doesn't make sense. We do this right for < 4 variants, but it still says "Count" always for > 4 variants

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Twixes do you know why the snapshot wouldn't show the full page / is there an option to snapshot the whole page?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, should show the whole page. Will look into it

@jurajmajerik jurajmajerik merged commit 0b18e79 into master Feb 6, 2024
79 checks passed
@jurajmajerik jurajmajerik deleted the fix/experiments-exposure-column branch February 6, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants