-
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-warehouse): query tabs #23958
Conversation
Size Change: +34 B (0%) 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. |
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 think it'd be really good to be able to preserve your tabs and the contents of your tabs within local storage!
createModel: () => { | ||
if (props.monaco) { | ||
const uri = props.monaco.Uri.parse((values.modelCount + 1).toString()) | ||
const model = props.monaco.editor.createModel('SELECT event FROM events LIMIT 100', props.language, uri) |
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.
Should we use the default query from frontend/src/queries/examples.ts
?
|
||
function QueryTab({ model, active, onClear, onClick }: QueryTabProps): JSX.Element { | ||
return ( | ||
<button |
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.
Should we use LemonTabs or something similar for 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.
avoided lemontabs as I wanted custom UI that was very much editor-tab like
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.
just updated the images in the PR description to match
onClear ? 'pl-3 pr-2' : 'px-4' | ||
)} | ||
> | ||
{'Query ' + model.path.split('/').pop()} |
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.
Will we allow people to name these at some point?
@Gilbert09 one more pass please on the state logic. I will add this behind a feature flag too as it's a significant implementation that should be tested more deeply |
onClick={() => | ||
saveAs( | ||
true, | ||
'Only the currently open query will be saved as an insight. All other draft queries will be discarded' | ||
) | ||
} |
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.
Not one for right now, but I wonder whether we can make this experience better somehow. Maybe opening the new insight in a new tab, or just having a toast with a link to the insight
…g into feat/data-pipeline-scenes
Hmm, why is this better than browser tabs? Tabs have come up at times in the past (@corywatilo can tell you a lot about it), but in the browser context, they easily make things more confusing by duplicating browser functionality. |
@Twixes maybe not the strongest point here but gotten this request internally and externally because it would make flipping between queries part of the posthog experience. I don't think people prefer to have multiple tabs of posthog open. Can dig more here |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated5 snapshot changes in total. 0 added, 5 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 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 updated3 snapshot changes in total. 0 added, 3 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. |
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Ben White <[email protected]>
Problem
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?