-
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
refactor(data-exploration): remove data exploration from editor filters #15497
refactor(data-exploration): remove data exploration from editor filters #15497
Conversation
📸 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 updated51 snapshot changes in total. 0 added, 51 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.
Looks good to me, though I'm worried about the skipped tests, as they tend to be left that way for longer than intended most of the time
@@ -325,7 +325,8 @@ describe('Dashboard', () => { | |||
savedInsights.checkInsightIsInListView(insightName) | |||
}) | |||
|
|||
it('can delete dashboard and delete the insights', () => { | |||
// TODO: this test works locally, just not in CI |
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.
Experience suggests that, with a comment like this, this will be un-skipped never (± 2 months). 😄
component = ( | ||
<DataTable query={query} setQuery={setQuery} context={queryContext} cachedResults={props.cachedResults} /> | ||
) | ||
} else if (isDataNode(query)) { | ||
component = <DataNode query={query} cachedResults={props.cachedResults} /> | ||
} else if (isInsightVizNode(query)) { | ||
component = <InsightViz query={query} setQuery={setQuery} context={queryContext} /> | ||
} else if (isInsightQueryNode(query)) { |
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.
This doesn't have legacy
in the name, is it right to remove this?
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.
Good spot 👀 ! It should be alright to remove this as it was only used by the "ad hoc" insight and I've checked plugins for usage of these components. Your comment led me to check the /debug page though, where the queries still exist and now I'm wondering wether we want to support rendering all source nodes as well. Will re-add this or remove the examples in a separate PR though.
Problem
The "view source" feature aka data exploration is rolled out to 100% of users, but we still have dead code lying around. See PostHog/meta#84 for an overview of outstanding tasks.
Changes
This PR
cachedInsight
ascachedResults
ofdataNodeLogic
inInsightViz
<AdHocInsight />
<InsightQuery />
<LegacyInsightQuery />
How did you test this code?