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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0efead2
[XY] Fixes the broken chart when an agg is placed in another axis and…
stratoula Dec 17, 2021
128adca
Fix yAxes ticks
stratoula Dec 17, 2021
8cc4c38
Fix test borkem due to the change of the implementation
stratoula Dec 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions src/plugins/vis_types/xy/public/config/get_config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"].

expect(config.yAxes[1].ticks?.formatter).toStrictEqual(config.aspects.y[1].formatter);
});

it('assigns the correct number of yAxes if the agg is hidden', () => {
// We have two axes but the one y dimension is hidden
const newVisParams = {
...visParamsWithTwoYAxes,
dimensions: {
...visParamsWithTwoYAxes.dimensions,
y: [
{
label: 'Average memory',
aggType: 'avg',
params: {},
accessor: 1,
format: {
id: 'number',
params: {},
},
},
],
},
};
const config = getConfig(visData, newVisParams);
expect(config.yAxes.length).toBe(1);
});
});
20 changes: 11 additions & 9 deletions src/plugins/vis_types/xy/public/config/get_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,17 @@ export function getConfig(
} = params;
const aspects = getAspects(table.columns, params.dimensions);
const tooltip = getTooltip(aspects, params);
const yAxes = params.valueAxes.map((a) => {
// find the correct aspect for each value axis
const aspectsIdx = params.seriesParams.findIndex((s) => s.valueAxis === a.id);
return getAxis<YScaleType>(
a,
params.grid,
aspects.y[aspectsIdx > -1 ? aspectsIdx : 0],
params.seriesParams
);

const yAxes: Array<AxisConfig<ScaleContinuousType>> = [];

params.dimensions.y.forEach((y) => {
const accessor = y.accessor;
const aspect = aspects.y.find(({ column }) => column === accessor);
const serie = params.seriesParams.find(({ data: { id } }) => id === aspect?.aggId);
const valueAxis = params.valueAxes.find(({ id }) => id === serie?.valueAxis);
if (aspect && valueAxis) {
yAxes.push(getAxis<YScaleType>(valueAxis, params.grid, aspect, params.seriesParams));
}
});

const rotation = getRotation(params.categoryAxes[0]);
Expand Down