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

[XY] Fixes the broken chart when an agg is placed in another axis and then is hidden #121488

Merged
merged 3 commits into from
Dec 17, 2021

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Dec 17, 2021

Summary

Closes #121268

There is a bug when you try to create a XY chart with two metrics on two y axes (one in the left and one in the right). If you hide one of the aggs, then the chart was failing with the error:
image

This PR fixes it:
image

Checklist

@stratoula stratoula changed the title [XY] Fixes the broken chart when an agg is placed in another axis and… [XY] Fixes the broken chart when an agg is placed in another axis and then is hidden Dec 17, 2021
@stratoula stratoula added Feature:XYAxis XY-Axis charts (bar, area, line) v7.16.2 v7.17.0 v8.0.0 v8.1.0 auto-backport Deprecated - use backport:version if exact versions are needed release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Dec 17, 2021
@stratoula stratoula marked this pull request as ready for review December 17, 2021 10:13
@stratoula stratoula requested a review from a team as a code owner December 17, 2021 10:13
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

@flash1293
Copy link
Contributor

There's still some issue with the right axis - it doesn't show any ticks or axis label:
Screenshot 2021-12-17 at 14 04 31

if i'm enabling both it starts working
Screenshot 2021-12-17 at 14 06 04

@stratoula
Copy link
Contributor Author

Thanx @flash1293 for pointing this out. I changed the implementation of computing the axis and I think that now all cases work fine.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM but I don't understand a change in an unrelated test

@@ -38,7 +38,31 @@ describe('getConfig', () => {
it('assigns the correct formatter per y axis', () => {
const config = getConfig(visData, visParamsWithTwoYAxes);
expect(config.yAxes.length).toBe(2);
expect(config.yAxes[0].ticks?.formatter).toStrictEqual(config.aspects.y[1].formatter);
expect(config.yAxes[1].ticks?.formatter).toStrictEqual(config.aspects.y[0].formatter);
expect(config.yAxes[0].ticks?.formatter).toStrictEqual(config.aspects.y[0].formatter);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this change? It seems like now another formatter is attached per axis but I can't see why that should happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the implementation of calculating the yAxes array. So in the old way (we were looping into the valueAxes array to compute it), the y Axes was : ["left-axis", "right-axis"]
With the current implementation (looping the dimensions.y) the yAxes looks like ["right-axis", "left-axis"].

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visTypeXy 60.4KB 60.6KB +142.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@stratoula stratoula merged commit e559b31 into elastic:main Dec 17, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 17, 2021
… then is hidden (elastic#121488)

* [XY] Fixes the broken chart when an agg is placed in another axis and then is hidden

* Fix yAxes ticks

* Fix test borkem due to the change of the implementation
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 17, 2021
… then is hidden (elastic#121488)

* [XY] Fixes the broken chart when an agg is placed in another axis and then is hidden

* Fix yAxes ticks

* Fix test borkem due to the change of the implementation
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Dec 17, 2021
… then is hidden (elastic#121488)

* [XY] Fixes the broken chart when an agg is placed in another axis and then is hidden

* Fix yAxes ticks

* Fix test borkem due to the change of the implementation
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0
7.17
7.16

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Dec 17, 2021
… then is hidden (#121488) (#121554)

* [XY] Fixes the broken chart when an agg is placed in another axis and then is hidden

* Fix yAxes ticks

* Fix test borkem due to the change of the implementation

Co-authored-by: Stratoula Kalafateli <[email protected]>
kibanamachine added a commit that referenced this pull request Dec 17, 2021
… then is hidden (#121488) (#121555)

* [XY] Fixes the broken chart when an agg is placed in another axis and then is hidden

* Fix yAxes ticks

* Fix test borkem due to the change of the implementation

Co-authored-by: Stratoula Kalafateli <[email protected]>
kibanamachine added a commit that referenced this pull request Dec 17, 2021
… then is hidden (#121488) (#121556)

* [XY] Fixes the broken chart when an agg is placed in another axis and then is hidden

* Fix yAxes ticks

* Fix test borkem due to the change of the implementation

Co-authored-by: Stratoula Kalafateli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:XYAxis XY-Axis charts (bar, area, line) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.16.2 v7.17.0 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot destructure property 'params' of 'undefined' as it is undefined.
5 participants