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: allow sorting Lifecycle graph bars #16542

Merged
merged 9 commits into from
Jul 31, 2023

Conversation

nategrift
Copy link
Contributor

My first PR on PostHog, feedback is greatly appreciated!

Problem

Enhancement from issue #14579

I want to be able to order the different bars on a lifecycle graph (i.e. new, resurrecting, or recurring users) so I can more easily compare what I care about

Changes

  • Added the ability to drag and drop the lifecycle filters around
  • Order of the filter is the order in how they will display on graph (both on dashboard and on insight screen)
  • The order persists and is saved to the insight so the user only has to change once.

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

How did you test this code?

  • Added tests for saving and fetching the saved lifecycle order
  • Checked that the order stays both on the insight screen and on the dashboard screen.
  • Checked existing lifecycles still work and new ones can be created without errors.

Screenshots of how it looks

Screenshot 2023-07-12 155830

@neilkakkar neilkakkar requested review from thmsobrmlr, pauldambra and mariusandra and removed request for pauldambra July 14, 2023 09:09
@thmsobrmlr
Copy link
Contributor

thmsobrmlr commented Jul 19, 2023

Hey @nategrift, thanks for your contribution to PostHog! Overall your PR looks very well made :)

After looking into the changes I'm inclined to reject the PR however. The reason being that I don't think the proposed functionality is useful enough to justify maintaining it (drag-and-drop being especially finicky in my experience).

More concretely I saw that dormant users will always be below the 0-line as negative numbers and I was asking myself why I'd want to re-order the remaining three lifecycles.

Based on the user request that this originates from (see below) I'm thinking changing the default order so that new users (blue) are on top is the thing we actually want to do here:

I love posthog! This is a VERY minor annoyance. I think for growth accounting graphs, the main thing that you want to track is the comparison of returning (green) and resurrecting (purple) since it’s the core of your users. But because they are positioned on top of new (blue), it makes it impossible to quickly compare visually. Personally, I think new (blue) should be at the top, and returning/resurrecting should be at the bottom. Does it make sense what I’m saying?

In it's current state the PR would also need to be adapted to a refactor that landed in master and there's an issue when dragging a lifecycle outside of the designated drop area.

drag-outside-drop.mov

@nategrift
Copy link
Contributor Author

@thmsobrmlr
That makes sense. Would you like me to remove all the added code except for the sorting to make sure the top is always new users(blue). If not feel free to close the pr! 😃

@thmsobrmlr
Copy link
Contributor

Very happy to merge this in with only code for the sorting to make sure the top is always new users

@nategrift
Copy link
Contributor Author

Okay, will do!

@nategrift
Copy link
Contributor Author

@thmsobrmlr Fixed! Only sorts the graphs to make sure new always shows at top of graph. Also moved the code from trendsLogic.ts to trendsDataLogic.ts as you removed it in PR #16576. Let me know if there is anything else you want changed. 😄

Copy link
Contributor

@thmsobrmlr thmsobrmlr left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up @nategrift!

Edit: We're fixing up a few CI issues before merging this in (just FYI).

@Twixes Twixes mentioned this pull request Jul 26, 2023
@thmsobrmlr thmsobrmlr merged commit 1f3d2d6 into PostHog:master Jul 31, 2023
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.

2 participants