Skip to content

Commit

Permalink
[Lens][Visualize] Removes wrong padding on the dashboard
Browse files Browse the repository at this point in the history
  • Loading branch information
stratoula committed Jun 20, 2023
1 parent 2fe276e commit dfb2d80
Show file tree
Hide file tree
Showing 7 changed files with 154 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 @@ -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 dfb2d80

Please sign in to comment.