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

refactor(insights): remove filter based card content #24050

Merged
merged 53 commits into from
Aug 5, 2024

Conversation

thmsobrmlr
Copy link
Contributor

@thmsobrmlr thmsobrmlr commented Jul 29, 2024

Problem

We have feature flag query-based-dashboard-cards that replaces filter based cards with their query based equivalent in most cases (i.e. in contexts where the feature flag works). We'd like to remove the legacy code and roll out query based cards for all occurances.

Changes

  • Removes the legacy FilterBasedCard component.
  • Fixes a bug where an insight with many breakdown values would show the "Load more breakdown values..." in the card view.

How did you test this code?

CI run and clicking around

@thmsobrmlr thmsobrmlr force-pushed the remove-filter-based-card-3 branch from bda3ca9 to 88dd5af Compare July 29, 2024 15:12
@PostHog PostHog deleted a comment from posthog-bot Jul 29, 2024
@PostHog PostHog deleted a comment from posthog-bot Jul 29, 2024
@PostHog PostHog deleted a comment from posthog-bot Jul 29, 2024
@PostHog PostHog deleted a comment from posthog-bot Jul 29, 2024
@PostHog PostHog deleted a comment from github-actions bot Jul 29, 2024
Copy link
Contributor

github-actions bot commented Jul 29, 2024

Size Change: +2 B (0%)

Total Size: 1.07 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.07 MB +2 B (0%)

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

  • chromium: 0 added, 6 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.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did manually export one insight of each kind and compared it against the previous version - couldn't replicate the shrinking as seen in these snapshot tests e.g. this is the lifecycle insight with the new query based card:
export-lifecycle

@thmsobrmlr thmsobrmlr force-pushed the remove-filter-based-card-3 branch from 42e387e to eb73f1c Compare July 30, 2024 07:02
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@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 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@PostHog PostHog deleted a comment from posthog-bot Jul 30, 2024
@PostHog PostHog deleted a comment from posthog-bot Jul 30, 2024
@PostHog PostHog deleted a comment from posthog-bot Jul 30, 2024
@PostHog PostHog deleted a comment from posthog-bot Jul 30, 2024
@PostHog PostHog deleted a comment from posthog-bot Jul 30, 2024
@PostHog PostHog deleted a comment from posthog-bot Jul 30, 2024
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

  • chromium: 0 added, 1 modified, 0 deleted (wasn't pushed!)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

# Conflicts:
#	frontend/__snapshots__/scenes-app-insights--funnel-top-to-bottom-edit--dark.png
import { urls } from 'scenes/urls'

describe('insights date picker', () => {
beforeEach(() => {
cy.visit(urls.insightNew())
cy.waitForNetworkIdle(300)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was much easier than mocking requests to /query/ and /query/:queryId, which I couldn't get working.

@thmsobrmlr thmsobrmlr marked this pull request as ready for review August 2, 2024 08:17
@thmsobrmlr thmsobrmlr requested a review from a team August 2, 2024 08:17
@thmsobrmlr thmsobrmlr changed the title refactor(insights): remove filter based card content (take 3) refactor(insights): remove filter based card content Aug 2, 2024
Copy link
Contributor

@skoob13 skoob13 left a comment

Choose a reason for hiding this comment

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

The PR looks good! I didn't notice any problems with cards, exports, or embeddings.

@thmsobrmlr thmsobrmlr merged commit ea98024 into master Aug 5, 2024
101 checks passed
@thmsobrmlr thmsobrmlr deleted the remove-filter-based-card-3 branch August 5, 2024 07:14
silentninja pushed a commit to silentninja/posthog that referenced this pull request Aug 8, 2024
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.

3 participants