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

Fix TS for vis_type_vislib #58345

Merged
merged 14 commits into from
Mar 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 0 additions & 6 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,6 @@ module.exports = {
'react-hooks/exhaustive-deps': 'off',
},
},
{
files: ['src/legacy/core_plugins/vis_type_vislib/**/*.{js,ts,tsx}'],
rules: {
'react-hooks/exhaustive-deps': 'off',
},
},
{
files: [
'src/legacy/core_plugins/vis_default_editor/public/components/controls/**/*.{ts,tsx}',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ function ValidationWrapper<T = unknown>({

useEffect(() => {
setValidity(isPanelValid);
}, [isPanelValid]);
}, [isPanelValid, setValidity]);

return <Component {...rest} setMultipleValidity={setValidityHandler} />;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ function CategoryAxisPanel(props: CategoryAxisPanelProps) {
};
setCategoryAxis(updatedAxis);
},
[setCategoryAxis]
[setCategoryAxis, axis]
);

const setPosition = useCallback(
Expand Down Expand Up @@ -89,7 +89,7 @@ function CategoryAxisPanel(props: CategoryAxisPanelProps) {
setValue={setAxis}
/>

{axis.show && <LabelOptions axis={axis} axesName="categoryAxes" index={0} {...props} />}
{axis.show && <LabelOptions axesName="categoryAxes" index={0} {...props} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you touched this, could you please also optimize the LabelOptions component ?
This component is only relied on several params, but accepts all of VisOptionsProps<BasicVislibParams>.
Could be decreased (and could be helpful for performance, since will not cause additional re-rendering):

<LabelOptions 
    axisLabels={axis.labels}
    axisFilterCheckboxName={`xAxisFilterLabelsCheckbox${axis.id}`}
    setAxisLabel={setAxisLabel} />

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, don't worry about this anymore!
I prepared a PR with refactoring based on your current pr:
#59135

</EuiPanel>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,9 +193,10 @@ describe('MetricsAxisOptions component', () => {
const updatedSeriesParams = [{ ...chart, data: { ...chart.data, label: agg.makeLabel() } }];
const updatedValues = [{ ...axis, title: { text: agg.makeLabel() } }];

expect(setValue).toHaveBeenCalledTimes(3);
expect(setValue).toHaveBeenNthCalledWith(2, SERIES_PARAMS, updatedSeriesParams);
expect(setValue).toHaveBeenNthCalledWith(3, VALUE_AXES, updatedValues);
expect(setValue).toHaveBeenCalledTimes(5);
expect(setValue).toHaveBeenNthCalledWith(3, SERIES_PARAMS, updatedSeriesParams);
expect(setValue).toHaveBeenNthCalledWith(5, SERIES_PARAMS, updatedSeriesParams);
expect(setValue).toHaveBeenNthCalledWith(4, VALUE_AXES, updatedValues);
});

it('should not set the custom title to match the value axis label when more than one agg exists for that axis', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,72 +89,85 @@ function MetricsAxisOptions(props: ValidationVisOptionsProps<BasicVislibParams>)
}
);

const updateAxisTitle = (seriesParams?: SeriesParam[]) => {
const series = seriesParams || stateParams.seriesParams;
const axes = cloneDeep(stateParams.valueAxes);
let isAxesChanged = false;
let lastValuesChanged = false;
const lastLabels = { ...lastCustomLabels };
const lastMatchingSeriesAgg = { ...lastSeriesAgg };

stateParams.valueAxes.forEach((axis, axisNumber) => {
let newCustomLabel = '';
const matchingSeries: IAggConfig[] = [];

series.forEach((serie, seriesIndex) => {
if ((axisNumber === 0 && !serie.valueAxis) || serie.valueAxis === axis.id) {
const aggByIndex = aggs.bySchemaName('metric')[seriesIndex];
matchingSeries.push(aggByIndex);
const updateAxisTitle = useCallback(
(seriesParams?: SeriesParam[]) => {
const series = seriesParams || stateParams.seriesParams;
let isAxesChanged = false;
let lastValuesChanged = false;
const lastLabels = { ...lastCustomLabels };
const lastMatchingSeriesAgg = { ...lastSeriesAgg };

const axes = stateParams.valueAxes.map((axis, axisNumber) => {
let newCustomLabel = '';
let updatedAxis;
const matchingSeries: IAggConfig[] = [];

series.forEach((serie, seriesIndex) => {
if ((axisNumber === 0 && !serie.valueAxis) || serie.valueAxis === axis.id) {
const aggByIndex = aggs.bySchemaName('metric')[seriesIndex];
matchingSeries.push(aggByIndex);
}
});

if (matchingSeries.length === 1) {
// if several series matches to the axis, axis title is set according to the first serie.
newCustomLabel = matchingSeries[0].makeLabel();
}
});

if (matchingSeries.length === 1) {
// if several series matches to the axis, axis title is set according to the first serie.
newCustomLabel = matchingSeries[0].makeLabel();
}

if (lastCustomLabels[axis.id] !== newCustomLabel && newCustomLabel !== '') {
const lastSeriesAggType = get(lastSeriesAgg, `${matchingSeries[0].id}.type`);
const lastSeriesAggField = get(lastSeriesAgg, `${matchingSeries[0].id}.field`);
const matchingSeriesAggType = get(matchingSeries, '[0]type.name', '');
const matchingSeriesAggField = get(matchingSeries, '[0]params.field.name', '');
if (lastCustomLabels[axis.id] !== newCustomLabel && newCustomLabel !== '') {
const lastSeriesAggType = get(lastSeriesAgg, `${matchingSeries[0].id}.type`);
const lastSeriesAggField = get(lastSeriesAgg, `${matchingSeries[0].id}.field`);
const matchingSeriesAggType = get(matchingSeries, '[0]type.name', '');
const matchingSeriesAggField = get(matchingSeries, '[0]params.field.name', '');

const aggTypeIsChanged = lastSeriesAggType !== matchingSeriesAggType;
const aggFieldIsChanged = lastSeriesAggField !== matchingSeriesAggField;
const aggTypeIsChanged = lastSeriesAggType !== matchingSeriesAggType;
const aggFieldIsChanged = lastSeriesAggField !== matchingSeriesAggField;

lastMatchingSeriesAgg[matchingSeries[0].id] = {
type: matchingSeriesAggType,
field: matchingSeriesAggField,
};
lastLabels[axis.id] = newCustomLabel;
lastValuesChanged = true;

if (
Object.keys(lastCustomLabels).length !== 0 &&
(aggTypeIsChanged ||
aggFieldIsChanged ||
axis.title.text === '' ||
lastCustomLabels[axis.id] === axis.title.text)
) {
// Override axis title with new custom label
axes[axisNumber] = {
...axis,
title: { ...axis.title, text: newCustomLabel },
lastMatchingSeriesAgg[matchingSeries[0].id] = {
type: matchingSeriesAggType,
field: matchingSeriesAggField,
};
isAxesChanged = true;
lastLabels[axis.id] = newCustomLabel;
lastValuesChanged = true;

if (
Object.keys(lastCustomLabels).length !== 0 &&
(aggTypeIsChanged ||
aggFieldIsChanged ||
axis.title.text === '' ||
lastCustomLabels[axis.id] === axis.title.text) &&
newCustomLabel !== axis.title.text
) {
// Override axis title with new custom label
updatedAxis = {
...axis,
title: { ...axis.title, text: newCustomLabel },
};
isAxesChanged = true;
}
}
}
});

if (isAxesChanged) {
setValue('valueAxes', axes);
}
return updatedAxis || axis;
});

if (lastValuesChanged) {
setLastSeriesAgg(lastMatchingSeriesAgg);
setLastCustomLabels(lastLabels);
}
};
if (isAxesChanged) {
setValue('valueAxes', axes);
}

if (lastValuesChanged) {
setLastSeriesAgg(lastMatchingSeriesAgg);
setLastCustomLabels(lastLabels);
}
},
[
aggs,
lastCustomLabels,
lastSeriesAgg,
setValue,
stateParams.seriesParams,
stateParams.valueAxes,
]
);

const onValueAxisPositionChanged = useCallback(
(index: number, value: ValueAxis['position']) => {
Expand All @@ -168,7 +181,7 @@ function MetricsAxisOptions(props: ValidationVisOptionsProps<BasicVislibParams>)
};
setValue('valueAxes', valueAxes);
},
[stateParams.valueAxes, getUpdatedAxisName, setValue]
[stateParams.valueAxes, setValue]
);

const onCategoryAxisPositionChanged = useCallback(
Expand Down Expand Up @@ -226,7 +239,7 @@ function MetricsAxisOptions(props: ValidationVisOptionsProps<BasicVislibParams>)
setValue('grid', { ...stateParams.grid, valueAxis: undefined });
}
},
[stateParams.seriesParams, stateParams.valueAxes, setValue]
[stateParams.seriesParams, stateParams.valueAxes, setValue, stateParams.grid]
);

const changeValueAxis: ChangeValueAxis = useCallback(
Expand All @@ -241,13 +254,13 @@ function MetricsAxisOptions(props: ValidationVisOptionsProps<BasicVislibParams>)

updateAxisTitle();
},
[addValueAxis, setParamByIndex]
[addValueAxis, setParamByIndex, updateAxisTitle]
);

const schemaName = vis.type.schemas.metrics[0].name;
const metrics = useMemo(() => {
const schemaName = vis.type.schemas.metrics[0].name;
return aggs.bySchemaName(schemaName);
}, [vis.type.schemas.metrics[0].name, aggs]);
}, [schemaName, aggs]);

const firstValueAxesId = stateParams.valueAxes[0].id;

Expand Down Expand Up @@ -278,7 +291,7 @@ function MetricsAxisOptions(props: ValidationVisOptionsProps<BasicVislibParams>)

setValue('seriesParams', updatedSeries);
updateAxisTitle(updatedSeries);
}, [metrics, firstValueAxesId]);
}, [metrics, firstValueAxesId, setValue, stateParams.seriesParams, updateAxisTitle]);

const visType = useMemo(() => {
const types = uniq(stateParams.seriesParams.map(({ type }) => type));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ function ValueAxesPanel(props: ValueAxesPanelProps) {
/>
</EuiToolTip>
),
[removeValueAxis]
[removeValueAxis, removeButtonTooltip]
);

const addButtonTooltip = useMemo(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ function ValueAxisOptions(props: ValueAxisOptionsParams) {
setValue={setValueAxisTitle}
/>

<LabelOptions axis={axis} axesName="valueAxes" index={index} {...props} />
<LabelOptions axesName="valueAxes" {...props} />
</>
) : (
<EuiSpacer size="xs" />
Expand Down Expand Up @@ -204,7 +204,6 @@ function ValueAxisOptions(props: ValueAxisOptionsParams) {
<>
<EuiSpacer size="m" />
<CustomExtentsOptions
axis={axis}
setValueAxisScale={setValueAxisScale}
setValueAxis={setValueAxis}
{...props}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ function GridPanel({ stateParams, setValue, hasHistogramAgg }: VisOptionsProps<B
if (hasHistogramAgg) {
setGrid('categoryLines', false);
}
}, [hasHistogramAgg]);
}, [hasHistogramAgg, setGrid]);

return (
<EuiPanel paddingSize="s">
Expand Down