-
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(data-exploration): replace trendsLogic with trendsDataLogic #16576
Conversation
6381a98
to
57b0f4f
Compare
4f90003
to
29edb00
Compare
📸 UI snapshots have been updated6 snapshot changes in total. 0 added, 6 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated7 snapshot changes in total. 0 added, 7 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 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 updated6 snapshot changes in total. 0 added, 6 modified, 0 deleted:
Triggered by this commit. |
1ee1de1
to
8a4d99c
Compare
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'm definitely not able to verify every changed line, but on a higher level it looks very good. Clicking around insights and dashboards, things worked as they should. So 👍 for getting this in.
Problem
Cleanup according to PostHog/meta#84.
Most importantly removing the reliance of anything updating an insight by changing
insightLogic.filters
enables a fix for the dashboard issue #16558.Changes
This PR:
How did you test this code?
Manual testing