feat: Sidebar cohorts and annotations movement #18200
Conversation
…Hog/posthog into feat/annotations-in-data-management
frontend/src/scenes/data-management/database/DatabaseTableList.tsx
Outdated
Show resolved
Hide resolved
📸 UI snapshots have been updated44 snapshot changes in total. 0 added, 44 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. |
There was a problem hiding this comment.
Looks great overall. I just want to bring up two important points before we merge this:
- We absolutely must point out to users that Annotations have moved. Probably just a closable tooltip on the Data Management item. This should only show up for users who signed up before Annotations have moved, and every user should only see this once (if they acknowledge it, the notice should never again show up for them, e.g. on other devices).
- There's also the plan of moving cohorts into persons and groups. I think it's less disruptive if both changes happen at once. Otherwise product direction will seem a bit more chaotic.
frontend/__snapshots__/scenes-app-data-management--ingestion-warnings.png
Outdated
Show resolved
Hide resolved
Agreed, just not sure where this would go? By "closeable tooltip" you mean something that immediately shows up pointing at the data management that they can then dismiss? If so - i agree and will try it out.
Totally happy with this. I guess we can merge this into a holding branch, whilst we do the same for Persons and cohorts and then get it all in together. I could also feature flag it but given that it affects scenes and things like that, my vote would just be to have one PR that solves both. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Yeah, I think a
Agreed. Pulling Cohorts into Persons & Groups is more tricky UX-wise though, because the way I see the hierarchy there is like this:
Thinking about it broader, Cohorts are to Persons what Actions are to Events. But the Event Explorer has its own navbar item, while Actions are stashed under Data Management. So perhaps in some way Cohorts belong to Data Management, and Persons & Groups should still be their own item? |
I think Cohorts definitely makes sense with Persons and Groups from a user perspective. There is definitely an argument for it being in data management as it is technically more like an Action, but I don't think people think that way (or at least it feels very off to me) |
|
agree it feels off as a user. One big reason I think why is the mechanism: An action can be seen as a group of events with filters, but here you're defining what the group should be and what the filters should be. So it's a lot more like an alias, and hence might make sense to be in data management. Cohorts on the other hand end up being a group of users as well, buuut they are a computation, and usually not an alias. It's more like saving the persons for a specific trend query, or computing users who've done X and Y and Z, where you don't know apriori who these users are. Feels much closer to insights in this sense. Although, some parts do fit into the action paradigm, but I think that's a very small part (like PostHog Team cohort) - which makes a bit more sense to be in data management. |
# Conflicts: # frontend/__snapshots__/scenes-app-surveys--survey-view.png # frontend/src/layout/navigation-3000/navigationLogic.tsx # frontend/src/scenes/data-management/DataManagementPageTabs.tsx
# Conflicts: # frontend/__snapshots__/scenes-app-notebooks--recordings-playlist.png # frontend/src/scenes/persons/Persons.tsx
📸 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. |
# Conflicts: # frontend/__snapshots__/scenes-app-notebooks--recordings-playlist.png
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
|
This issue has 2043 words at 33 comments. Issues this long are hard to read or contribute to, and tend to take very long to reach a conclusion. Instead, why not:
Is this issue intended to be sprawling? Consider adding label |
📸 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 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 updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
# Conflicts: # frontend/__snapshots__/scenes-app-insights--trends-line-edit.png
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Problem
The sidebar is crowded to say the least. We want to consolidate a couple of things, one of them being Annotations -> Data Management which is what this PR does as well as Cohorts -> Persons.
Changes
It was a bit messy so I don't think we should do this as 3000-only, but rather make the change already.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?