[Lens][Visualize] Removes wrong padding on the dashboard (#159992)

## Summary

Closes https://github.com/elastic/kibana/issues/159942

If the height of a partition chart exceeds 1000px paddings are added,
reducing the chart size.
This is caused due to this piece of code
https://github.com/elastic/kibana/pull/122420

This was added for the aggbased editor to reduce a bit the pie size
(otherwise it was taking the full container size and the pie was huge)

Although we want this, we don't want this to be applied in dashboards or
lens editor. This PR is fixing this by adding the paddings only on the
agg based editor level

In agg based editor
<img width="651" alt="image"
src="48ac6fdd-43e3-46f5-8818-d40334678fce">

Dashboard with very tall treemap, no paddings
<img width="933" alt="image"
src="8787d6ab-887c-4c8d-8419-2c2d5659f2c1">



### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
This commit is contained in:
Stratoula Kalafateli 2023-06-20 16:02:48 +03:00 committed by GitHub
parent 66e87e63e9
commit c0e43dbf28
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 155 additions and 17 deletions

View file

@ -6,5 +6,10 @@
* Side Public License, v 1.
*/
export { extractContainerType, extractVisualizationType, getOverridesFor } from './utils';
export {
extractContainerType,
extractVisualizationType,
getOverridesFor,
isOnAggBasedEditor,
} from './utils';
export type { Simplify, MakeOverridesSerializable } from './types';

View file

@ -6,7 +6,7 @@
* Side Public License, v 1.
*/
import { getOverridesFor } from './utils';
import { getOverridesFor, isOnAggBasedEditor } from './utils';
describe('Overrides utilities', () => {
describe('getOverridesFor', () => {
@ -31,3 +31,77 @@ describe('Overrides utilities', () => {
});
});
});
describe('isOnAggBasedEditor', () => {
it('should return false if is on dashboard', () => {
const context = {
type: 'dashboard',
description: 'test',
child: {
type: 'lens',
name: 'lnsPie',
id: 'd8bb29a7-13a4-43fa-a162-d7705050bb6c',
description: 'test',
url: '/gdu/app/lens#/edit_by_value',
},
};
expect(isOnAggBasedEditor(context)).toEqual(false);
});
it('should return false if is on editor but lens', () => {
const context = {
type: 'application',
description: 'test',
child: {
type: 'lens',
name: 'lnsPie',
id: 'd8bb29a7-13a4-43fa-a162-d7705050bb6c',
description: 'test',
url: '/gdu/app/lens#/edit_by_value',
},
};
expect(isOnAggBasedEditor(context)).toEqual(false);
});
it('should return false if is on dashboard but agg_based', () => {
const context = {
type: 'dashboard',
description: 'test',
child: {
type: 'agg_based',
name: 'pie',
id: 'd8bb29a7-13a4-43fa-a162-d7705050bb6c',
description: 'test',
url: '',
},
};
expect(isOnAggBasedEditor(context)).toEqual(false);
});
it('should return true if is on editor but agg_based', () => {
const context = {
type: 'application',
description: 'test',
child: {
type: 'agg_based',
name: 'pie',
id: 'd8bb29a7-13a4-43fa-a162-d7705050bb6c',
description: 'test',
url: '',
},
};
expect(isOnAggBasedEditor(context)).toEqual(true);
});
it('should return false if child is missing', () => {
const context = {
type: 'application',
description: 'test',
};
expect(isOnAggBasedEditor(context)).toEqual(false);
});
it('should return false if context is missing', () => {
expect(isOnAggBasedEditor()).toEqual(false);
});
});

View file

@ -20,6 +20,31 @@ export const extractContainerType = (context?: KibanaExecutionContext): string |
}
};
/* Function to identify if the pie is rendered inside the aggBased editor
Context comes with this format
{
type: 'dashboard', // application for lens, agg based charts
description: 'test',
child: {
type: 'lens', // agg_based for legacy editor
name: 'pie',
id: 'id',
description: 'test',
url: '',
},
}; */
export const isOnAggBasedEditor = (context?: KibanaExecutionContext): boolean => {
if (context) {
return Boolean(
context.type &&
context.type === 'application' &&
context.child &&
context.child.type === 'agg_based'
);
}
return false;
};
export const extractVisualizationType = (context?: KibanaExecutionContext): string | undefined => {
if (context) {
const recursiveGet = (item: KibanaExecutionContext): KibanaExecutionContext | undefined => {

View file

@ -83,6 +83,7 @@ describe('PartitionVisComponent', function () {
data: dataPluginMock.createStartContract(),
fieldFormats: fieldFormatsServiceMock.createStartContract(),
},
hasOpenedOnAggBasedEditor: false,
};
});

View file

@ -93,6 +93,7 @@ export type PartitionVisComponentProps = Omit<
palettesRegistry: PaletteRegistry;
services: Pick<StartDeps, 'data' | 'fieldFormats'>;
columnCellValueActions: ColumnCellValueActions;
hasOpenedOnAggBasedEditor: boolean;
};
const PartitionVisComponent = (props: PartitionVisComponentProps) => {
@ -105,6 +106,7 @@ const PartitionVisComponent = (props: PartitionVisComponentProps) => {
syncColors,
interactive,
overrides,
hasOpenedOnAggBasedEditor,
} = props;
const visParams = useMemo(() => filterOutConfig(visType, preVisParams), [preVisParams, visType]);
const chartTheme = props.chartsThemeService.useChartsTheme();
@ -148,7 +150,7 @@ const PartitionVisComponent = (props: PartitionVisComponentProps) => {
const [showLegend, setShowLegend] = useState<boolean>(() => showLegendDefault());
const showToggleLegendElement = props.uiState !== undefined;
const [chartIsLoaded, setChartIsLoaded] = useState<boolean>(false);
const [containerDimensions, setContainerDimensions] = useState<
undefined | PieContainerDimensions
>();
@ -156,12 +158,14 @@ const PartitionVisComponent = (props: PartitionVisComponentProps) => {
const parentRef = useRef<HTMLDivElement>(null);
useEffect(() => {
if (parentRef && parentRef.current) {
// chart should be loaded to compute the dimensions
// otherwise the height is set to 0
if (parentRef && parentRef.current && chartIsLoaded) {
const parentHeight = parentRef.current!.getBoundingClientRect().height;
const parentWidth = parentRef.current!.getBoundingClientRect().width;
setContainerDimensions({ width: parentWidth, height: parentHeight });
}
}, [parentRef]);
}, [chartIsLoaded, parentRef]);
useEffect(() => {
const legendShow = showLegendDefault();
@ -172,6 +176,7 @@ const PartitionVisComponent = (props: PartitionVisComponentProps) => {
(isRendered: boolean = true) => {
if (isRendered) {
props.renderComplete();
setChartIsLoaded(true);
}
},
[props]
@ -363,8 +368,16 @@ const PartitionVisComponent = (props: PartitionVisComponentProps) => {
) as Partial<SettingsProps>;
const themeOverrides = useMemo(
() => getPartitionTheme(visType, visParams, chartTheme, containerDimensions, rescaleFactor),
[visType, visParams, chartTheme, containerDimensions, rescaleFactor]
() =>
getPartitionTheme(
visType,
visParams,
chartTheme,
containerDimensions,
rescaleFactor,
hasOpenedOnAggBasedEditor
),
[visType, visParams, chartTheme, containerDimensions, rescaleFactor, hasOpenedOnAggBasedEditor]
);
const fixedViewPort = document.getElementById('app-fixed-viewport');

View file

@ -21,7 +21,11 @@ import { withSuspense } from '@kbn/presentation-util-plugin/public';
import { KibanaThemeProvider } from '@kbn/kibana-react-plugin/public';
import { METRIC_TYPE } from '@kbn/analytics';
import { getColumnByAccessor } from '@kbn/visualizations-plugin/common/utils';
import { extractContainerType, extractVisualizationType } from '@kbn/chart-expressions-common';
import {
extractContainerType,
extractVisualizationType,
isOnAggBasedEditor,
} from '@kbn/chart-expressions-common';
import { VisTypePieDependencies } from '../plugin';
import { PARTITION_VIS_RENDERER_NAME } from '../../common/constants';
import { CellValueAction, GetCompatibleCellValueActions } from '../types';
@ -110,6 +114,8 @@ export const getPartitionVisRenderer: (
plugins.charts.palettes.getPalettes(),
]);
const hasOpenedOnAggBasedEditor = isOnAggBasedEditor(handlers.getExecutionContext());
render(
<I18nProvider>
<KibanaThemeProvider theme$={core.theme.theme$}>
@ -128,6 +134,7 @@ export const getPartitionVisRenderer: (
syncColors={syncColors}
columnCellValueActions={columnCellValueActions}
overrides={overrides}
hasOpenedOnAggBasedEditor={hasOpenedOnAggBasedEditor}
/>
</div>
</KibanaThemeProvider>

View file

@ -144,9 +144,16 @@ const runPieDonutWaffleTestSuites = (chartType: ChartTypes, visParams: Partition
});
});
it('should return adjusted padding settings if dimensions are specified', () => {
it('should return adjusted padding settings if dimensions are specified and is on aggBased editor', () => {
const specifiedDimensions = { width: 2000, height: 2000 };
const theme = getPartitionTheme(chartType, visParams, chartTheme, specifiedDimensions);
const theme = getPartitionTheme(
chartType,
visParams,
chartTheme,
specifiedDimensions,
undefined,
true
);
expect(theme).toEqual({
...getStaticThemeOptions(chartTheme, visParams),
@ -233,7 +240,6 @@ const runPieDonutWaffleTestSuites = (chartType: ChartTypes, visParams: Partition
expect(theme).toEqual({
...getStaticThemeOptions(chartTheme, visParams),
chartPaddings: { top: 500, bottom: 500, left: 500, right: 500 },
partition: {
...getStaticThemePartition(chartTheme, visParams),
outerSizeRatio: rescaleFactor,
@ -263,7 +269,6 @@ const runPieDonutWaffleTestSuites = (chartType: ChartTypes, visParams: Partition
expect(theme).toEqual({
...getStaticThemeOptions(chartTheme, vParams),
chartPaddings: { top: 500, bottom: 500, left: 500, right: 500 },
partition: {
...getStaticThemePartition(chartTheme, vParams),
outerSizeRatio: 0.5,
@ -285,7 +290,6 @@ const runPieDonutWaffleTestSuites = (chartType: ChartTypes, visParams: Partition
expect(theme).toEqual({
...getStaticThemeOptions(chartTheme, vParams),
chartPaddings: { top: 500, bottom: 500, left: 500, right: 500 },
partition: {
...getStaticThemePartition(chartTheme, vParams),
linkLabel: linkLabelWithEnoughSpace(vParams),
@ -420,7 +424,14 @@ const runTreemapMosaicTestSuites = (chartType: ChartTypes, visParams: PartitionV
it('should return fullfilled padding settings if dimensions are specified', () => {
const specifiedDimensions = { width: 2000, height: 2000 };
const theme = getPartitionTheme(chartType, visParams, chartTheme, specifiedDimensions);
const theme = getPartitionTheme(
chartType,
visParams,
chartTheme,
specifiedDimensions,
undefined,
true
);
expect(theme).toEqual({
...getStaticThemeOptions(chartTheme, visParams),

View file

@ -26,7 +26,8 @@ type GetThemeFn = (
visParams: PartitionVisParams,
chartTheme: RecursivePartial<Theme>,
dimensions?: PieContainerDimensions,
rescaleFactor?: number
rescaleFactor?: number,
hasOpenedOnAggBasedEditor?: boolean
) => PartialTheme;
type GetPieDonutWaffleThemeFn = (
@ -118,12 +119,13 @@ export const getPartitionTheme: GetThemeFn = (
visParams,
chartTheme,
dimensions,
rescaleFactor = 1
rescaleFactor = 1,
hasOpenedOnAggBasedEditor
) => {
// On small multiples we want the labels to only appear inside
const isSplitChart = Boolean(visParams.dimensions.splitColumn || visParams.dimensions.splitRow);
const paddingProps: PartialTheme | null =
dimensions && !isSplitChart
dimensions && !isSplitChart && hasOpenedOnAggBasedEditor
? {
chartPaddings: {
top: ((1 - Math.min(1, MAX_SIZE / dimensions?.height)) / 2) * dimensions?.height,