From c0e43dbf2834ea526ef43cd5d366e19434cd1cdc Mon Sep 17 00:00:00 2001 From: Stratoula Kalafateli Date: Tue, 20 Jun 2023 16:02:48 +0300 Subject: [PATCH] [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 image Dashboard with very tall treemap, no paddings image ### 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 --- src/plugins/chart_expressions/common/index.ts | 7 +- .../chart_expressions/common/utils.test.ts | 76 ++++++++++++++++++- src/plugins/chart_expressions/common/utils.ts | 25 ++++++ .../partition_vis_component.test.tsx | 1 + .../components/partition_vis_component.tsx | 23 ++++-- .../partition_vis_renderer.tsx | 9 ++- .../public/utils/get_partition_theme.test.ts | 23 ++++-- .../public/utils/get_partition_theme.ts | 8 +- 8 files changed, 155 insertions(+), 17 deletions(-) diff --git a/src/plugins/chart_expressions/common/index.ts b/src/plugins/chart_expressions/common/index.ts index 4373260657909e..0983b1ed28d4d7 100644 --- a/src/plugins/chart_expressions/common/index.ts +++ b/src/plugins/chart_expressions/common/index.ts @@ -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'; diff --git a/src/plugins/chart_expressions/common/utils.test.ts b/src/plugins/chart_expressions/common/utils.test.ts index 2ed71e9a17b92f..48519c9f6f1a97 100644 --- a/src/plugins/chart_expressions/common/utils.test.ts +++ b/src/plugins/chart_expressions/common/utils.test.ts @@ -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); + }); +}); diff --git a/src/plugins/chart_expressions/common/utils.ts b/src/plugins/chart_expressions/common/utils.ts index 2966532c44117b..db2e564efc4b3e 100644 --- a/src/plugins/chart_expressions/common/utils.ts +++ b/src/plugins/chart_expressions/common/utils.ts @@ -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 => { diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/components/partition_vis_component.test.tsx b/src/plugins/chart_expressions/expression_partition_vis/public/components/partition_vis_component.test.tsx index 5eb48cfab6cd53..c935ce847e40e1 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/components/partition_vis_component.test.tsx +++ b/src/plugins/chart_expressions/expression_partition_vis/public/components/partition_vis_component.test.tsx @@ -83,6 +83,7 @@ describe('PartitionVisComponent', function () { data: dataPluginMock.createStartContract(), fieldFormats: fieldFormatsServiceMock.createStartContract(), }, + hasOpenedOnAggBasedEditor: false, }; }); diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/components/partition_vis_component.tsx b/src/plugins/chart_expressions/expression_partition_vis/public/components/partition_vis_component.tsx index 94590ff1645557..4ce300a7d9bb99 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/components/partition_vis_component.tsx +++ b/src/plugins/chart_expressions/expression_partition_vis/public/components/partition_vis_component.tsx @@ -93,6 +93,7 @@ export type PartitionVisComponentProps = Omit< palettesRegistry: PaletteRegistry; services: Pick; 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(() => showLegendDefault()); const showToggleLegendElement = props.uiState !== undefined; - + const [chartIsLoaded, setChartIsLoaded] = useState(false); const [containerDimensions, setContainerDimensions] = useState< undefined | PieContainerDimensions >(); @@ -156,12 +158,14 @@ const PartitionVisComponent = (props: PartitionVisComponentProps) => { const parentRef = useRef(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; 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'); diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/expression_renderers/partition_vis_renderer.tsx b/src/plugins/chart_expressions/expression_partition_vis/public/expression_renderers/partition_vis_renderer.tsx index 056ba6b7136ce9..2379096796639f 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/expression_renderers/partition_vis_renderer.tsx +++ b/src/plugins/chart_expressions/expression_partition_vis/public/expression_renderers/partition_vis_renderer.tsx @@ -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( @@ -128,6 +134,7 @@ export const getPartitionVisRenderer: ( syncColors={syncColors} columnCellValueActions={columnCellValueActions} overrides={overrides} + hasOpenedOnAggBasedEditor={hasOpenedOnAggBasedEditor} /> diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/utils/get_partition_theme.test.ts b/src/plugins/chart_expressions/expression_partition_vis/public/utils/get_partition_theme.test.ts index 345d6ce068d0c3..31d025ac0310f8 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/utils/get_partition_theme.test.ts +++ b/src/plugins/chart_expressions/expression_partition_vis/public/utils/get_partition_theme.test.ts @@ -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), diff --git a/src/plugins/chart_expressions/expression_partition_vis/public/utils/get_partition_theme.ts b/src/plugins/chart_expressions/expression_partition_vis/public/utils/get_partition_theme.ts index edb1aaea64aad0..3714cac911829c 100644 --- a/src/plugins/chart_expressions/expression_partition_vis/public/utils/get_partition_theme.ts +++ b/src/plugins/chart_expressions/expression_partition_vis/public/utils/get_partition_theme.ts @@ -26,7 +26,8 @@ type GetThemeFn = ( visParams: PartitionVisParams, chartTheme: RecursivePartial, 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,