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

fix dashboard grid item performs 2 DOM queries every render #199390

Merged
merged 15 commits into from
Nov 18, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,7 @@ packages/core/plugins/core-plugins-server-mocks @elastic/kibana-core
packages/core/preboot/core-preboot-server @elastic/kibana-core
packages/core/preboot/core-preboot-server-internal @elastic/kibana-core
packages/core/preboot/core-preboot-server-mocks @elastic/kibana-core
packages/core/rendering/core-rendering-browser @elastic/kibana-core
packages/core/rendering/core-rendering-browser-internal @elastic/kibana-core
packages/core/rendering/core-rendering-browser-mocks @elastic/kibana-core
packages/core/rendering/core-rendering-server-internal @elastic/kibana-core
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@
"@kbn/core-preboot-server": "link:packages/core/preboot/core-preboot-server",
"@kbn/core-preboot-server-internal": "link:packages/core/preboot/core-preboot-server-internal",
"@kbn/core-provider-plugin": "link:test/plugin_functional/plugins/core_provider_plugin",
"@kbn/core-rendering-browser": "link:packages/core/rendering/core-rendering-browser",
"@kbn/core-rendering-browser-internal": "link:packages/core/rendering/core-rendering-browser-internal",
"@kbn/core-rendering-server-internal": "link:packages/core/rendering/core-rendering-server-internal",
"@kbn/core-root-browser-internal": "link:packages/core/root/core-root-browser-internal",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import type { I18nStart } from '@kbn/core-i18n-browser';
import type { OverlayStart } from '@kbn/core-overlays-browser';
import type { ThemeServiceStart } from '@kbn/core-theme-browser';
import { KibanaRootContextProvider } from '@kbn/react-kibana-context-root';
import { APP_FIXED_VIEWPORT_ID } from '@kbn/core-rendering-browser';
nreese marked this conversation as resolved.
Show resolved Hide resolved
import { AppWrapper } from './app_containers';

interface StartServices {
Expand Down Expand Up @@ -68,7 +69,7 @@ export class RenderingService {
{/* The App Wrapper outside of the fixed headers that accepts custom class names from apps */}
<AppWrapper chromeVisible$={chrome.getIsVisible$()}>
{/* Affixes a div to restrict the position of charts tooltip to the visible viewport minus the header */}
<div id="app-fixed-viewport" />
<div id={APP_FIXED_VIEWPORT_ID} />

{/* The actual plugin/app */}
{appComponent}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
"@kbn/core-analytics-browser-mocks",
"@kbn/core-analytics-browser",
"@kbn/core-i18n-browser",
"@kbn/core-theme-browser"
"@kbn/core-theme-browser",
"@kbn/core-rendering-browser"
],
"exclude": [
"target/**/*",
Expand Down
4 changes: 4 additions & 0 deletions packages/core/rendering/core-rendering-browser/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# @kbn/core-rendering-browser

This package contains the types and implementation for Core's browser-side rendering service.

10 changes: 10 additions & 0 deletions packages/core/rendering/core-rendering-browser/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

export { APP_FIXED_VIEWPORT_ID, useAppFixedViewport } from './src';
nreese marked this conversation as resolved.
Show resolved Hide resolved
14 changes: 14 additions & 0 deletions packages/core/rendering/core-rendering-browser/jest.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

module.exports = {
preset: '@kbn/test',
rootDir: '../../../..',
roots: ['<rootDir>/packages/core/rendering/core-rendering-browser'],
};
5 changes: 5 additions & 0 deletions packages/core/rendering/core-rendering-browser/kibana.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"type": "shared-browser",
"id": "@kbn/core-rendering-browser",
"owner": "@elastic/kibana-core"
}
7 changes: 7 additions & 0 deletions packages/core/rendering/core-rendering-browser/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"name": "@kbn/core-rendering-browser",
"private": true,
"version": "1.0.0",
"author": "Kibana Core",
"license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0"
}
10 changes: 10 additions & 0 deletions packages/core/rendering/core-rendering-browser/src/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

export { APP_FIXED_VIEWPORT_ID, useAppFixedViewport } from './use_app_fixed_viewport';
nreese marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

import { useRef } from 'react';

export const APP_FIXED_VIEWPORT_ID = 'app-fixed-viewport';

export function useAppFixedViewport() {
nreese marked this conversation as resolved.
Show resolved Hide resolved
const ref = useRef(document.getElementById(APP_FIXED_VIEWPORT_ID) ?? undefined);
return ref.current;
}
19 changes: 19 additions & 0 deletions packages/core/rendering/core-rendering-browser/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"extends": "../../../../tsconfig.base.json",
"compilerOptions": {
"outDir": "target/types",
"types": [
"jest",
"node",
"react"
]
},
"include": [
"**/*.ts",
"**/*.tsx",
],
"kbn_references": [],
"exclude": [
"target/**/*",
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
} from '@kbn/expressions-plugin/public';
import type { FieldFormat } from '@kbn/field-formats-plugin/common';
import { getOverridesFor } from '@kbn/chart-expressions-common';
import { useAppFixedViewport } from '@kbn/core-rendering-browser';
import { consolidateMetricColumns } from '../../common/utils';
import { DEFAULT_PERCENT_DECIMALS } from '../../common/constants';
import {
Expand Down Expand Up @@ -385,7 +386,7 @@ const PartitionVisComponent = (props: PartitionVisComponentProps) => {
[visType, visParams, containerDimensions, rescaleFactor, hasOpenedOnAggBasedEditor]
);

const fixedViewPort = document.getElementById('app-fixed-viewport');
const fixedViewPort = useAppFixedViewport();

const legendPosition = visParams.legendPosition ?? Position.Right;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
"@kbn/chart-expressions-common",
"@kbn/cell-actions",
"@kbn/react-kibana-context-render",
"@kbn/core-rendering-browser",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import {
} from '@kbn/visualizations-plugin/common/constants';
import { PersistedState } from '@kbn/visualizations-plugin/public';
import { getOverridesFor, ChartSizeSpec } from '@kbn/chart-expressions-common';
import { useAppFixedViewport } from '@kbn/core-rendering-browser';
import type {
FilterEvent,
BrushEvent,
Expand Down Expand Up @@ -232,6 +233,7 @@ export function XYChart({
const chartRef = useRef<Chart>(null);
const chartBaseTheme = chartsThemeService.useChartsBaseTheme();
const darkMode = chartsThemeService.useDarkMode();
const appFixedViewport = useAppFixedViewport();
const filteredLayers = getFilteredLayers(layers);
const layersById = filteredLayers.reduce<Record<string, CommonXYLayerConfig>>(
(hashMap, layer) => ({ ...hashMap, [layer.layerId]: layer }),
Expand Down Expand Up @@ -767,7 +769,7 @@ export function XYChart({
>
<Chart ref={chartRef} {...getOverridesFor(overrides, 'chart')}>
<Tooltip<Record<string, string | number>, XYChartSeriesIdentifier>
boundary={document.getElementById('app-fixed-viewport') ?? undefined}
boundary={appFixedViewport}
headerFormatter={
!args.detailedTooltip && xAxisColumn
? ({ value }) => (
Expand Down
1 change: 1 addition & 0 deletions src/plugins/chart_expressions/expression_xy/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"@kbn/es-query",
"@kbn/cell-actions",
"@kbn/react-kibana-context-render",
"@kbn/core-rendering-browser",
],
"exclude": [
"target/**/*",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ export function FiltersNotificationPopover({ api }: { api: FiltersNotificationAc
}
}, [api, setDisableEditButton]);

const [hasLockedHoverActions, dataViews, parentViewMode] = useBatchedOptionalPublishingSubjects(
api.hasLockedHoverActions$,
const [dataViews, parentViewMode] = useBatchedOptionalPublishingSubjects(
api.parentApi?.dataViews,
getViewModeSubject(api ?? undefined)
);
Expand All @@ -77,7 +76,7 @@ export function FiltersNotificationPopover({ api }: { api: FiltersNotificationAc
onClick={() => {
setIsPopoverOpen(!isPopoverOpen);
if (apiCanLockHoverActions(api)) {
api?.lockHoverActions(!hasLockedHoverActions);
api?.lockHoverActions(!api.hasLockedHoverActions$.value);
}
}}
data-test-subj={`embeddablePanelNotification-${api.uuid}`}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,21 @@ import { Layout, Responsive as ResponsiveReactGridLayout } from 'react-grid-layo
import { ViewMode } from '@kbn/embeddable-plugin/public';

import { useBatchedPublishingSubjects } from '@kbn/presentation-publishing';
import { useAppFixedViewport } from '@kbn/core-rendering-browser';
import { DashboardPanelState } from '../../../../common';
import { DashboardGridItem } from './dashboard_grid_item';
import { useDashboardGridSettings } from './use_dashboard_grid_settings';
import { useDashboardApi } from '../../../dashboard_api/use_dashboard_api';
import { getPanelLayoutsAreEqual } from '../../state/diffing/dashboard_diffing_utils';
import { DASHBOARD_GRID_HEIGHT, DASHBOARD_MARGIN_SIZE } from '../../../dashboard_constants';

export const DashboardGrid = ({ viewportWidth }: { viewportWidth: number }) => {
export const DashboardGrid = ({
dashboardContainer,
viewportWidth,
}: {
dashboardContainer?: HTMLElement;
viewportWidth: number;
}) => {
const dashboardApi = useDashboardApi();

const [animatePanelTransforms, expandedPanelId, focusedPanelId, panels, useMargins, viewMode] =
Expand All @@ -51,6 +58,8 @@ export const DashboardGrid = ({ viewportWidth }: { viewportWidth: number }) => {
}
}, [expandedPanelId]);

const appFixedViewport = useAppFixedViewport();

const panelsInOrder: string[] = useMemo(() => {
return Object.keys(panels).sort((embeddableIdA, embeddableIdB) => {
const panelA = panels[embeddableIdA];
Expand All @@ -72,6 +81,8 @@ export const DashboardGrid = ({ viewportWidth }: { viewportWidth: number }) => {
const type = panels[embeddableId].type;
return (
<DashboardGridItem
appFixedViewport={appFixedViewport}
dashboardContainer={dashboardContainer}
data-grid={panels[embeddableId].gridData}
key={embeddableId}
id={embeddableId}
Expand All @@ -82,7 +93,14 @@ export const DashboardGrid = ({ viewportWidth }: { viewportWidth: number }) => {
/>
);
});
}, [expandedPanelId, panels, panelsInOrder, focusedPanelId]);
}, [
appFixedViewport,
dashboardContainer,
expandedPanelId,
panels,
panelsInOrder,
focusedPanelId,
]);

const onLayoutChange = useCallback(
(newLayout: Array<Layout & { i: string }>) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import { embeddableService, presentationUtilService } from '../../../services/ki
type DivProps = Pick<React.HTMLAttributes<HTMLDivElement>, 'className' | 'style' | 'children'>;

export interface Props extends DivProps {
appFixedViewport?: HTMLElement;
dashboardContainer?: HTMLElement;
id: DashboardPanelState['explicitInput']['id'];
index?: number;
type: DashboardPanelState['type'];
Expand All @@ -35,6 +37,8 @@ export interface Props extends DivProps {
export const Item = React.forwardRef<HTMLDivElement, Props>(
(
{
appFixedViewport,
dashboardContainer,
expandedPanelId,
focusedPanelId,
id,
Expand Down Expand Up @@ -92,10 +96,8 @@ export const Item = React.forwardRef<HTMLDivElement, Props>(
}
}, [id, dashboardApi, scrollToPanelId, highlightPanelId, ref, blurPanel]);

const dashboardContainerTopOffset =
(document.querySelector('.dashboardContainer') as HTMLDivElement)?.offsetTop || 0;
const globalNavTopOffset =
(document.querySelector('#app-fixed-viewport') as HTMLDivElement)?.offsetTop || 0;
const dashboardContainerTopOffset = dashboardContainer?.offsetTop || 0;
const globalNavTopOffset = appFixedViewport?.offsetTop || 0;

const focusStyles = blurPanel
? css`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export const useDebouncedWidthObserver = (skipDebounce = false, wait = 100) => {
return { ref, width };
};

export const DashboardViewport = () => {
export const DashboardViewport = ({ dashboardContainer }: { dashboardContainer?: HTMLElement }) => {
const dashboardApi = useDashboardApi();
const [hasControls, setHasControls] = useState(false);
const [
Expand Down Expand Up @@ -160,7 +160,9 @@ export const DashboardViewport = () => {
otherwise, there is a race condition where the panels can end up being squashed
TODO only render when dashboardInitialized
*/}
{viewportWidth !== 0 && <DashboardGrid viewportWidth={viewportWidth} />}
{viewportWidth !== 0 && (
<DashboardGrid dashboardContainer={dashboardContainer} viewportWidth={viewportWidth} />
)}
</div>
</div>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ export class DashboardContainer
coreStart={{ chrome: coreServices.chrome, customBranding: coreServices.customBranding }}
>
<DashboardContext.Provider value={this as DashboardApi}>
<DashboardViewport />
<DashboardViewport dashboardContainer={this.domNode} />
</DashboardContext.Provider>
</ExitFullScreenButtonKibanaProvider>
</KibanaRenderContextProvider>,
Expand Down
1 change: 1 addition & 0 deletions src/plugins/dashboard/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
"@kbn/core-custom-branding-browser-mocks",
"@kbn/core-mount-utils-browser",
"@kbn/visualization-utils",
"@kbn/core-rendering-browser",
],
"exclude": ["target/**/*"]
}
2 changes: 2 additions & 0 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,8 @@
"@kbn/core-preboot-server-mocks/*": ["packages/core/preboot/core-preboot-server-mocks/*"],
"@kbn/core-provider-plugin": ["test/plugin_functional/plugins/core_provider_plugin"],
"@kbn/core-provider-plugin/*": ["test/plugin_functional/plugins/core_provider_plugin/*"],
"@kbn/core-rendering-browser": ["packages/core/rendering/core-rendering-browser"],
"@kbn/core-rendering-browser/*": ["packages/core/rendering/core-rendering-browser/*"],
"@kbn/core-rendering-browser-internal": ["packages/core/rendering/core-rendering-browser-internal"],
"@kbn/core-rendering-browser-internal/*": ["packages/core/rendering/core-rendering-browser-internal/*"],
"@kbn/core-rendering-browser-mocks": ["packages/core/rendering/core-rendering-browser-mocks"],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
} from '@elastic/charts';
import { useEuiTheme } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { useAppFixedViewport } from '@kbn/core-rendering-browser';
import { useBaseChartTheme } from '../../../../../../hooks/use_base_chart_theme';
import { BAR_HEIGHT } from './constants';
import { WaterfallChartChartContainer, WaterfallChartTooltip } from './styles';
Expand Down Expand Up @@ -86,6 +87,8 @@ export const WaterfallBarChart = ({
const handleProjectionClick = useMemo(() => onProjectionClick, [onProjectionClick]);
const memoizedTickFormat = useCallback(tickFormat, [tickFormat]);

const appFixedViewport = useAppFixedViewport();

return (
<WaterfallChartChartContainer
height={getChartHeight(chartData)}
Expand All @@ -96,7 +99,7 @@ export const WaterfallBarChart = ({
<Tooltip
// this is done to prevent the waterfall tooltip from rendering behind Kibana's
// stacked header when the user highlights an item at the top of the chart
boundary={document.getElementById('app-fixed-viewport') ?? undefined}
boundary={appFixedViewport}
customTooltip={CustomTooltip}
/>
<Settings
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
"@kbn/ebt-tools",
"@kbn/alerting-types",
"@kbn/core-chrome-browser",
"@kbn/core-rendering-browser",
"@kbn/index-lifecycle-management-common-shared"
],
"exclude": ["target/**/*"]
Expand Down
Loading