-
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(web-analytics): Add mini-version of the retention cohort analysis #18586
Conversation
fde40b0
to
3cfb4b8
Compare
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.
Couple of small things but overall looks good!
Would be cool if we could get my big refactor in first... #18570
7ffe706
to
c7cb871
Compare
Size Change: 0 B Total Size: 2.01 MB ℹ️ View Unchanged
|
📸 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. |
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. The one exception is the "small layout" thing. I think this is the kind of property that over time becomes hard to manage and rather the component itself should just do this with container queries
[InsightType.RETENTION]?: { | ||
hideLineGraph?: boolean | ||
hideSizeColumn?: boolean | ||
useSmallLayout?: boolean |
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 is the only property I don't think makes sense.
We should do this with container queries given that it looks nicer regardless of the setting, it's more based on the container size anyways which applies no matter where it is rendered right?
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 don't 100% agree, you might want to use this with a large container to show a ton of data, e.g. if you had 54 columns to show weekly retention over a year. We could do something smart like try to figure out whether what the user has asked for fits inside the container, and use the small layout if it doesn't, but I don't want to overengineer 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.
Fair enough. Then lets go with it for now
2c028e9
to
f48eec8
Compare
Problem
No retention cohort analysis on web analytics, and the UI that does exist is hard to fit in a small space.
Changes
I added some options to the retention table, to make it work better in a smaller space.
iPhone SE
iPad
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
How did you test this code?