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

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

Merged
merged 2 commits into from Jun 20, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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