-
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
fix(insights): polishing of multiple breakdowns in Trends #23649
Conversation
Size Change: 0 B Total Size: 1.07 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
…g/posthog into fix/multiple-breakdowns-polishing
📸 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.
Heya @skoob13, thanks for the follow up here! Overall the changes look good. I left a couple of comments inline. In general I think it would be good to dry up the code a bit, as its hard to understand as is. Might also be worth splitting into separate PRs in the future.
frontend/__snapshots__/components-cards-insight-details--trends-world-map--dark.png
Outdated
Show resolved
Hide resolved
frontend/src/lib/components/Cards/InsightCard/InsightDetails.tsx
Outdated
Show resolved
Hide resolved
frontend/src/lib/components/Cards/InsightCard/InsightDetails.tsx
Outdated
Show resolved
Hide resolved
frontend/src/scenes/insights/filters/BreakdownFilter/taxonomicBreakdownFilterLogic.ts
Show resolved
Hide resolved
…g/posthog into fix/multiple-breakdowns-polishing
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.
Thanks for the follow-up @skoob13 ! This looks good now, with only one change that I think we should make before merging in: The current implementation would break the insight details for the other implementation of multi-breakdowns. I don't think we need to wait for the refactor to the same format and could instead just handle both formats gracefully there.
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 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. |
📸 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. |
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.
Some CI unhappiness (looks unrelated), otherwise ready to go in 👍
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
…-breakdowns-polishing
…g/posthog into fix/multiple-breakdowns-polishing
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Problem
Fixes the following bugs related to the multiple breakdowns:
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Unit and manual testing