feat: Added option to display multiple Y-axes for Trends related line charts #28316
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
Based on the provided files, I'll summarize the key changes for adding multiple Y-axes support to line graphs:
This PR adds the ability to display multiple Y-axes in trend line graphs, making it easier to compare series with significantly different scales. Here are the key changes:
- Added new
showMultipleYAxesboolean option in the trends filter schema with corresponding UI toggle in the Options menu - Implemented Y-axis generation logic in
LineGraph.tsxthat alternates between left/right positioning for multiple axes - Modified dataset processing to assign different Y-axis IDs to each series when multiple axes are enabled
- Added support for maintaining individual axis scales, formatting, and goal lines across multiple Y-axes
- Ensured compatibility with existing features like logarithmic scales and annotations while adding the new Y-axes functionality
The implementation provides a clean solution to the issue of comparing trends with vastly different event volumes by allowing each series to have its own appropriately scaled axis.
💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!
15 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -93,6 +93,7 @@ describe('queryNodeToFilter', () => { | |||
| showLabelsOnSeries: true, | |||
| showPercentStackView: true, | |||
| yAxisScaleType: 'log10', | |||
| showMultipleYAxes: false, | |||
There was a problem hiding this comment.
logic: showMultipleYAxes is set in the input query but missing from the expected output filters (lines 107-131). This will cause the feature to fail silently.
| @@ -57,6 +58,7 @@ export function InsightDisplayConfig(): JSX.Element { | |||
| supportsPercentStackView, | |||
| supportsResultCustomizationBy, | |||
| yAxisScaleType, | |||
| showMultipleYAxes, | |||
There was a problem hiding this comment.
style: showMultipleYAxes is destructured but not used in any conditional rendering logic beyond the options count. Consider if this should affect other display behaviors.
| const { showMultipleYAxes } = useValues(insightVizDataLogic(insightProps)) | ||
| const { updateInsightFilter } = useActions(insightVizDataLogic(insightProps)) |
There was a problem hiding this comment.
style: insightVizDataLogic is instantiated twice - consider storing the result in a variable to avoid potential duplicate instantiations
| const { showMultipleYAxes } = useValues(insightVizDataLogic(insightProps)) | |
| const { updateInsightFilter } = useActions(insightVizDataLogic(insightProps)) | |
| const logic = insightVizDataLogic(insightProps) | |
| const { showMultipleYAxes } = useValues(logic) | |
| const { updateInsightFilter } = useActions(logic) |
frontend/src/scenes/insights/EditorFilters/ShowMultipleYAxesFilter.tsx
Outdated
Show resolved
Hide resolved
Sriram-bk
changed the title
|
Size Change: +5 B (0%) Total Size: 1.17 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is such a useful (and long overdue) change, great work getting this out so quickly! Works so smoothly 🎉
- Just from a UX perspective, maybe we disable grid lines when this is enabled as inconsistent grid line spacing is hard to comprehend?
- Also should we limit to just 2 axes? (not a blocker) When there's more than 2 series, it adds multiple axes and with the grid lines it becomes hard to read the graph. Not sure if there's a better way to stack axes.
|
@anirudhpillai Thanks for the call out on the grid lines! Updated to hide grid lines if we're displaying multiple Y-axes. To your point about limiting to 2, I think we should leave this as is. I do agree that it get's really confusing, but I'd like to default to being more flexible. If we find that users are using this with more than 2 series, then we can also rely on their feedback to come up with a better way to stack and represent those use cases. |
Sriram-bk
force-pushed
the
sri/add-option-for-multiple-y-axes-to-line-graphs
branch
from
047b04f to
0a81744
Compare
There was a problem hiding this comment.
After talking to Sri in person, the plan is to ship this as is, which provides useful functionality to people who want to graph two graphs, or graph more than 2 graphs and care more about relative movements than absolute numbers.
We will then look into improving it as necessary based on people's requirements (grouping graphs, for example)
Sriram-bk
deleted the
sri/add-option-for-multiple-y-axes-to-line-graphs
branch
Problem
Currently a trend insight with two or more series is unreadable/not useful if one series contains significantly more events than the others.
This PR adds an option to assign a second (or multiple) Y-axis, so the trends of two or more series can be easily compared even if the volume of events is very different.
Closes #17898
Changes
Before
After
Options Selector
👉 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?
Tested locally.