feat(retention): graph UX updates #29270
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR enhances the retention graph UX with multiple visualization and display options for better dashboard integration.
- Added
RetentionOptionscomponent that separates retention calculation settings from display controls - Created
RetentionChartPickerallowing users to switch between line and bar chart visualizations - Implemented
RetentionDashboardDisplayPickerwith options to show table-only, graph-only, or both in dashboards - Replaced checkbox with
RetentionCumulativeButtonusing segmented controls with improved tooltips - Modified
RetentionContainerto conditionally render components based on dashboard display preferences
20 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
frontend/src/scenes/insights/filters/RetentionCumulativeButton.tsx
Outdated
Show resolved
Hide resolved
frontend/src/scenes/insights/filters/RetentionCumulativeButton.tsx
Outdated
Show resolved
Hide resolved
| const showLineGraph = | ||
| !vizSpecificOptions?.hideLineGraph && | ||
| (!inCardView || | ||
| (!!retentionFilter?.dashboardDisplay && // for backwards compatibility as we were hiding the graph on dashboards before adding this property | ||
| retentionFilter?.dashboardDisplay !== RetentionDashboardDisplayType.TableOnly)) | ||
|
|
There was a problem hiding this comment.
logic: The showLineGraph logic has a potential issue with the double negative (!vizSpecificOptions?.hideLineGraph). When inCardView is true but retentionFilter.dashboardDisplay is undefined (for existing dashboards), the graph will be hidden. This matches the comment about backwards compatibility, but might be confusing for users upgrading.
|
Size Change: +813 B (+0.01%) Total Size: 9.73 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated22 snapshot changes in total. 0 added, 22 modified, 0 deleted:
Triggered by this commit. |
…into feat/retention-graph-ux
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Cool thing!
Some remarks:
- Please add frontend only settings to https://github.com/PostHog/posthog/blob/master/frontend/src/scenes/insights/utils/queryUtils.ts#L132-L151 and https://github.com/PostHog/posthog/blob/master/posthog/schema_helpers.py#L58-L81.
- I accepted a couple of greptile's suggestions regarding grammar spelling.
- It looks like the numbers change in visual regression tests. Is the code in
retentionTableLogiccorrect? - Schema changes are backwards compatible, so no issue there.
There was a problem hiding this comment.
Seems odd to me that the numbers would change in visual regression tests.
There was a problem hiding this comment.
Updating retention reference updates the table as well now (previously only changed the line graph). This would mean some numbers changed, but is there a way to tell if this snapshot was created with a different retention reference?
There was a problem hiding this comment.
The snapshots come from storybook and the query for that is mocked in https://github.com/PostHog/posthog/tree/master/frontend/src/mocks/fixtures/api/projects/team_id/insights.
Mocks still have filters in them, could that cause an issue as well? - Happy to pair on this, if easier.
📸 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. |
…into feat/retention-graph-ux
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated12 snapshot changes in total. 0 added, 12 modified, 0 deleted:
Triggered by this commit. |
Problem
Closes #23911
Lot of UX changes highlighted below
Changes
Option to show retention graph in dashboards
Option for bar chart instead of line graph (will add more enhancements to bar chart in future PRs)
New section for 'Retention calculation options', buttons around chart only control display configs (eventually will move mean to the side as well, once Sri's changes are merged in)
Selecting retention reference actually updates the table now (previously only changed the line graph)
Added tooltips for cumulative retention
👉 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?