-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Lens]Do not enable histogram mode for multiple un-stacked bar series #78525
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
Jenkins, test this |
Seems like a few things are not right yet, let me know when you need help fixing them. About the logic itself: This wasn’t explicitly stated in the issue, but the chart should still use histogram mode even if there are multiple series as long as it’s in stacked mode. Histogram mode means the width of the bars in the chart corresponds to the interval the value takes up on the x axis. This does work fine for stacked bars (because stacking only changes height, not width), but it doesn’t when multiple bars are shown next to each other for the same x axis value because they would overlap. That’s why we have to disable histogram mode for explicitly unstacked bar charts if they have multiple series |
That makes sense. My understanding from the issue description,
was that we need to disable histogram mode in either of two cases: the chart has multiple layers or multiple metrics, irrespective of the Based on your explanation, we need to disable histogram mode in case of |
Sounds better - actually, now that I’m thinking about it a bit more, there is something else to consider. As long as the multiple series are area or line series, we still can use histogram mode as those don’t mess with the x axis width. So we can’t use chartHasMoreThanOneSeries directly but we have to check whether there are multiple unstacked bar series specifically. |
@flash1293 I've pushed a commit with the logic to disable histogram mode for One existing test - |
@PavithraCP Thanks for updating! I think there is one more case missing though - your logic is using chartHasMoreThanOneSeries and disables histogram mode if the current series is an unstacked bar and there is more than one series. The problem is the following case: you have an unstacked bar series (without split) together with a line series or area series in a second layer. In your PR this would disable histogram mode (because there is more than one series in the chart), but actually it would work fine because the line/area series can be displayed on top of the single bar series without interfering with the x axis width. So instead of relying on the chartHasMoreThanOneSeries variable you need to check whether there is more than one bar series by iterating through all of the layers and checking whether there is more than one bar series. The rest of the logic looks right, it’s basically replacing your usage of chartHasMoreThanOneSeries by “chartHasMoreThanOneBarSeries” Also, could you update your PR by merging in current master from the elastic/kibana branch? Thanks again for sticking with it 💚 |
@flash1293 Implemented the suggested change and re-based the branch to latest master. Also ran the new logic against the bug faced in the issue and checked if the behavior is as expected. I've attached the screenshot in the PR summary. Please let me know if the logic looks good. Thanks! |
@elasticmachine merge upstream |
Jenkins, test this |
Pinging @elastic/kibana-app (Team:KibanaApp) |
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.
Tested in Chrome and works as expected. LGTM if the build comes out green.
💚 Build SucceededMetrics [docs]async chunks size
History
To update your PR or re-run it, just comment with: |
I'm going to merge, thanks for your contribution 💚 |
Summary
Fix for Issue #72243
Do not enable histogram mode for multiple bar series in case of un-stacked bar charts. Please refer the discussion below for more details.
The output of the bug in #72243 after applying this fix is as follows:
Checklist
Delete any items that are not applicable to this PR.
For maintainers