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

feat: replace open in query builder with new insight page #14709

Merged
merged 48 commits into from
Mar 20, 2023

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Mar 13, 2023

Problem

Now that the insight scene can be used to edit insights, let's open query editing in a new insight

see #13927

Changes

does that

2023-03-13 14 27 54

After #14688 we can teach the insight scene to open the correct tab depending on if a hogql or non-hogql query is being presented

Will follow-up with switching insight viz tabs to text editing...

How did you test this code?

👀 locally

@thmsobrmlr
Copy link
Contributor

Not sure what's in scope for this PR yet, but I'd expect that clicking the button selects the query tab and closes the drawer. Code wise all looks good 👍

Not for now, just wanted to mention: It would be great to pass the query not by url, but through some router state, which I think isn't possible yet. I suspect we'll run into URL length limit with this otherwise.

@mariusandra
Copy link
Collaborator

I was thinking we should get rid of the drawer altogether. In my imagination the "<>" button on "event explorer" directly opens a new insight. 🤔

Then, I'd hide that button when editing an insight itself. For some views, we could have the "<>" edit query drawer open, but I'd hide that under "..." -> "edit source".

pauldambra added a commit that referenced this pull request Mar 13, 2023
@thmsobrmlr
Copy link
Contributor

thmsobrmlr commented Mar 13, 2023

I'm not a fan of the <> either, so +1 for getting rid of that here.

Opens up the question of how we'd edit the query for an insight then though. In a previous design decision we opted for making the tabs in insights independent of each other, so that with data exploration each tab will open in a "blank" state. It would be strange if the query tab would behave differently.

I could imagine a small toggle in the top-right corner of editor filters that switches from filters to a query view and vice-versa.

Edit: Something like this, that's maybe just shown when hovering the editor filters.
edit as code

@pauldambra
Copy link
Member Author

Opens up the question of how we'd edit the query for an insight then

Yep, and at the moment we check "is this an insight viz" and if so you can't get to the query tab! So, we'd deffo need a different route.

I like the idea of swapping between text-based editing and click-based editing 🤔

@mariusandra
Copy link
Collaborator

Summarising this, what about a flow where:

  1. Everywhere we have any embedded "query", be it the events explorer, the persons table, a groups table, a random embedded insight in an experiment, a persons modal, or whatever... then instead of <>, it'll have a :open_in_new_window: icon, which will open it as a new insight.
  2. When in the "insights" view, the same icon will be replaced with a <> "edit source" button, which for now will just open the drawer to edit the JSON. For now. Perhaps we'll replace it with a non-drawer later.
  3. The insights view has tabs, one for each "insight type". We'll add two more: "SQL" and "Data table". One for HogQL queries (DataTableNode -> HogQLQuery), one for any other data table. When you open anything "as a new insight", we'll directly open the right tab. Be it "Trends", "SQL" or "Data table".
  4. Any unknown query types can create a new tab... or just open the data table.
  5. In the future we'll convert data tables to "universal search" tables where you can also select the data source (events? persons? anything!)... but we don't care about that today...

@pauldambra
Copy link
Member Author

updated the gif in the description

2023-03-13 14 27 54

@pauldambra pauldambra mentioned this pull request Mar 13, 2023
24 tasks
@pauldambra
Copy link
Member Author

Didn't see your comment @mariusandra, so I have a flow

  1. Everywhere we have any embedded "query" that is not an insight viz then instead of <>, it'll have a :edit database: icon, which will open it as a new insight.
  2. When in that "insights" view, we are editing as text so it opens in "query" tab (as we add more tabs we can direct people to the correct one instead e.g. "SQL" for hogQL.

I think that at the moment we need the fall back to "this is a query" and then we can start to open the correct tab as we solidify this

The line between an InsightViz and a Query is pretty strict at the moment.

Hmmm, but I did remove the drawer entirely here which gives you no escape hatch for insights 🤔

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

pauldambra added a commit that referenced this pull request Mar 13, 2023
* feat: hogql editor pane

* schema build

* type fix

* hide reload in insight placement

* push the query tab for just staff users (while things are in flux)

* user can be null

* remove Q

* put insight query tab behind a flag

* fix loading from drop down: requires #14709

* less is more

* quick and dirty show correct editor panel until #14709 is in

* don't hide the reload button

* copy url change in from #14709 to fix new insight dropdown in this PR

* add beta tag to tab label

* only show query tab to posthog staff

* Update UI snapshots for `chromium` (2)

* Update UI snapshots for `chromium` (2)

* fix things missed while rushing 🙈

* slightly better viewbox

* don't show SQL as an option in new insights if the feature flag isn't on

* very important change

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
@mariusandra
Copy link
Collaborator

I think this is very good! I'd ✅ , but Thomas's point might need a look though :/.

@pauldambra
Copy link
Member Author

@mariusandra current plan here is

then

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@mariusandra mariusandra merged commit 1f1d2c2 into master Mar 20, 2023
@mariusandra mariusandra deleted the feat/replace-open-in-with-insight-page branch March 20, 2023 23:44
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.

4 participants