Skip to content

Commit

Permalink
[Lens][Visualize] Removes wrong padding on the dashboard (#159992)
Browse files Browse the repository at this point in the history
## Summary

Closes #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
#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="https://github.com/elastic/kibana/assets/17003240/48ac6fdd-43e3-46f5-8818-d40334678fce">

Dashboard with very tall treemap, no paddings
<img width="933" alt="image"
src="https://github.com/elastic/kibana/assets/17003240/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
  • Loading branch information
stratoula committed Jun 20, 2023
1 parent 66e87e6 commit c0e43db
Show file tree
Hide file tree
Showing 8 changed files with 155 additions and 17 deletions.
7 changes: 6 additions & 1 deletion src/plugins/chart_expressions/common/index.ts
Expand Up @@ -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';
76 changes: 75 additions & 1 deletion src/plugins/chart_expressions/common/utils.test.ts
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { getOverridesFor } from './utils';
import { getOverridesFor, isOnAggBasedEditor } from './utils';

describe('Overrides utilities', () => {
describe('getOverridesFor', () => {
Expand All @@ -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);
});
});
25 changes: 25 additions & 0 deletions src/plugins/chart_expressions/common/utils.ts
Expand Up @@ -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 => {
Expand Down
Expand Up @@ -83,6 +83,7 @@ describe('PartitionVisComponent', function () {
data: dataPluginMock.createStartContract(),
fieldFormats: fieldFormatsServiceMock.createStartContract(),
},
hasOpenedOnAggBasedEditor: false,
};
});

Expand Down
Expand Up @@ -93,6 +93,7 @@ export type PartitionVisComponentProps = Omit<
palettesRegistry: PaletteRegistry;
services: Pick<StartDeps, 'data' | 'fieldFormats'>;
columnCellValueActions: ColumnCellValueActions;
hasOpenedOnAggBasedEditor: boolean;
};

const PartitionVisComponent = (props: PartitionVisComponentProps) => {
Expand All @@ -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();
Expand Down Expand Up @@ -148,20 +150,22 @@ 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
>();

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();
Expand All @@ -172,6 +176,7 @@ const PartitionVisComponent = (props: PartitionVisComponentProps) => {
(isRendered: boolean = true) => {
if (isRendered) {
props.renderComplete();
setChartIsLoaded(true);
}
},
[props]
Expand Down Expand Up @@ -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');
Expand Down
Expand Up @@ -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';
Expand Down Expand Up @@ -110,6 +114,8 @@ export const getPartitionVisRenderer: (
plugins.charts.palettes.getPalettes(),
]);

const hasOpenedOnAggBasedEditor = isOnAggBasedEditor(handlers.getExecutionContext());

render(
<I18nProvider>
<KibanaThemeProvider theme$={core.theme.theme$}>
Expand All @@ -128,6 +134,7 @@ export const getPartitionVisRenderer: (
syncColors={syncColors}
columnCellValueActions={columnCellValueActions}
overrides={overrides}
hasOpenedOnAggBasedEditor={hasOpenedOnAggBasedEditor}
/>
</div>
</KibanaThemeProvider>
Expand Down
Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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),
Expand Down Expand Up @@ -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),
Expand Down
Expand Up @@ -26,7 +26,8 @@ type GetThemeFn = (
visParams: PartitionVisParams,
chartTheme: RecursivePartial<Theme>,
dimensions?: PieContainerDimensions,
rescaleFactor?: number
rescaleFactor?: number,
hasOpenedOnAggBasedEditor?: boolean
) => PartialTheme;

type GetPieDonutWaffleThemeFn = (
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit c0e43db

Please sign in to comment.