Skip to content

Commit

Permalink
fix: use passed size in pixel without waiting for resizeObserver (#2270)
Browse files Browse the repository at this point in the history
This change bypasses the need for one ResizeObserver event when passing chart size in pixels.
This avoids double re-renders with different sizes in the case of a datum + size change from the caller component.
  • Loading branch information
markov00 authored Jan 31, 2024
1 parent 0bf586b commit f9c11fc
Show file tree
Hide file tree
Showing 12 changed files with 32 additions and 152 deletions.
Binary file not shown.
5 changes: 3 additions & 2 deletions packages/charts/api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,7 @@ export class Chart extends React_2.Component<ChartProps, ChartState> {
// (undocumented)
componentDidMount(): void;
// (undocumented)
componentDidUpdate({ title, description }: Readonly<ChartProps>): void;
componentDidUpdate({ title, description, size }: Readonly<ChartProps>): void;
// (undocumented)
componentWillUnmount(): void;
// (undocumented)
Expand Down Expand Up @@ -2656,7 +2656,7 @@ export const Settings: (props: SFProps<SettingsSpec, keyof (typeof settingsBuild
// Warning: (ae-forgotten-export) The symbol "BuildProps" needs to be exported by the entry point index.d.ts
//
// @public (undocumented)
export const settingsBuildProps: BuildProps<SettingsSpec, "id" | "chartType" | "specType", "debug" | "locale" | "rotation" | "ariaLabelHeadingLevel" | "ariaUseDefaultSummary" | "legendPosition" | "flatLegend" | "legendMaxDepth" | "legendSize" | "showLegend" | "showLegendExtra" | "baseTheme" | "rendering" | "animateData" | "externalPointerEvents" | "pointBuffer" | "resizeDebounce" | "pointerUpdateTrigger" | "brushAxis" | "minBrushDelta" | "allowBrushingLastHistogramBin", "ariaLabel" | "xDomain" | "ariaDescription" | "ariaDescribedBy" | "ariaLabelledBy" | "ariaTableCaption" | "theme" | "legendAction" | "legendColorPicker" | "legendStrategy" | "onLegendItemClick" | "customLegend" | "onLegendItemMinusClick" | "onLegendItemOut" | "onLegendItemOver" | "onLegendItemPlusClick" | "orderOrdinalBinsBy" | "debugState" | "onProjectionClick" | "onElementClick" | "onElementOver" | "onElementOut" | "onBrushEnd" | "onPointerUpdate" | "onResize" | "onRenderChange" | "onWillRender" | "onProjectionAreaChange" | "onAnnotationClick" | "pointerUpdateDebounce" | "roundHistogramBrushValues" | "noResults" | "legendSort", never>;
export const settingsBuildProps: BuildProps<SettingsSpec, "id" | "chartType" | "specType", "debug" | "locale" | "rotation" | "ariaLabelHeadingLevel" | "ariaUseDefaultSummary" | "legendPosition" | "flatLegend" | "legendMaxDepth" | "legendSize" | "showLegend" | "showLegendExtra" | "baseTheme" | "rendering" | "animateData" | "externalPointerEvents" | "pointBuffer" | "pointerUpdateTrigger" | "brushAxis" | "minBrushDelta" | "allowBrushingLastHistogramBin", "ariaLabel" | "xDomain" | "ariaDescription" | "ariaDescribedBy" | "ariaLabelledBy" | "ariaTableCaption" | "theme" | "legendAction" | "legendColorPicker" | "legendStrategy" | "onLegendItemClick" | "customLegend" | "onLegendItemMinusClick" | "onLegendItemOut" | "onLegendItemOver" | "onLegendItemPlusClick" | "orderOrdinalBinsBy" | "debugState" | "onProjectionClick" | "onElementClick" | "onElementOver" | "onElementOut" | "onBrushEnd" | "onPointerUpdate" | "onResize" | "onRenderChange" | "onWillRender" | "onProjectionAreaChange" | "onAnnotationClick" | "resizeDebounce" | "pointerUpdateDebounce" | "roundHistogramBrushValues" | "noResults" | "legendSort", never>;

// @public (undocumented)
export type SettingsProps = ComponentProps<typeof Settings>;
Expand Down Expand Up @@ -2709,6 +2709,7 @@ export interface SettingsSpec extends Spec, LegendSpec {
pointerUpdateTrigger: PointerUpdateTrigger;
// (undocumented)
rendering: Rendering;
// @deprecated
resizeDebounce?: number;
// (undocumented)
rotation: Rotation;
Expand Down
1 change: 0 additions & 1 deletion packages/charts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
"react-redux": "^7.1.0",
"redux": "^4.0.4",
"reselect": "^4.0.0",
"resize-observer-polyfill": "^1.5.1",
"ts-debounce": "^4.0.0",
"utility-types": "^3.10.0",
"uuid": "^9",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ exports[`Chart should render the legend name test 1`] = `
</ChartStatusComponent>
</Connect(ChartStatusComponent)>
<Connect(Resizer)>
<Resizer resizeDebounce={10} onResize={[undefined]} updateParentDimensions={[Function (anonymous)]}>
<Resizer onResize={[undefined]} updateParentDimensions={[Function (anonymous)]}>
<div className="echChartResizer" />
</Resizer>
</Connect(Resizer)>
Expand Down
12 changes: 9 additions & 3 deletions packages/charts/src/components/chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ import { getElementZIndex } from './portal/utils';
import { Colors } from '../common/colors';
import { LegendPositionConfig, PointerEvent } from '../specs';
import { SpecsParser } from '../specs/specs_parser';
import { updateChartTitles } from '../state/actions/chart_settings';
import { updateChartTitles, updateParentDimensions } from '../state/actions/chart_settings';
import { onExternalPointerEvent } from '../state/actions/events';
import { onComputedZIndex } from '../state/actions/z_index';
import { chartStoreReducer, GlobalChartState } from '../state/chart_state';
import { getChartThemeSelector } from '../state/selectors/get_chart_theme';
import { getInternalIsInitializedSelector, InitStatus } from '../state/selectors/get_internal_is_intialized';
import { getLegendConfigSelector } from '../state/selectors/get_legend_config_selector';
import { ChartSize, getChartSize } from '../utils/chart_size';
import { ChartSize, getChartSize, getFixedChartSize } from '../utils/chart_size';
import { LayoutDirection } from '../utils/common';
import { LIGHT_THEME } from '../utils/themes/light_theme';

Expand Down Expand Up @@ -121,10 +121,16 @@ export class Chart extends React.Component<ChartProps, ChartState> {
this.unsubscribeToStore();
}

componentDidUpdate({ title, description }: Readonly<ChartProps>) {
componentDidUpdate({ title, description, size }: Readonly<ChartProps>) {
if (title !== this.props.title || description !== this.props.description) {
this.chartStore.dispatch(updateChartTitles(this.props.title, this.props.description));
}
const prevChartSize = getChartSize(size);
const newChartSize = getFixedChartSize(this.props.size);
// if the size is specified in pixels then update directly the store
if (newChartSize && (newChartSize.width !== prevChartSize.width || newChartSize.height !== prevChartSize.height)) {
this.chartStore.dispatch(updateParentDimensions({ ...newChartSize, top: 0, left: 0 }));
}
}

getPNGSnapshot(
Expand Down
54 changes: 6 additions & 48 deletions packages/charts/src/components/chart_resizer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,13 @@
import React, { RefObject } from 'react';
import { connect } from 'react-redux';
import { Dispatch, bindActionCreators } from 'redux';
import ResizeObserver from 'resize-observer-polyfill';

import { DEFAULT_RESIZE_DEBOUNCE } from '../specs/constants';
import { ResizeListener } from '../specs/settings';
import { updateParentDimensions } from '../state/actions/chart_settings';
import { GlobalChartState } from '../state/chart_state';
import { getSettingsSpecSelector } from '../state/selectors/get_settings_spec';
import { isFiniteNumber } from '../utils/common';
import { debounce, DebouncedFunction } from '../utils/debounce';

interface ResizerStateProps {
resizeDebounce: number;
onResize?: ResizeListener;
}

Expand All @@ -32,44 +27,26 @@ type ResizerProps = ResizerStateProps & ResizerDispatchProps;
type ResizeFn = (entries: ResizeObserverEntry[]) => void;

class Resizer extends React.Component<ResizerProps> {
private initialResizeComplete = false;

private readonly containerRef: RefObject<HTMLDivElement>;

private ro: ResizeObserver;

private animationFrameID: number;

private onResizeDebounced?: ResizeFn | DebouncedFunction<Parameters<ResizeFn>, ResizeFn>;

constructor(props: ResizerProps) {
super(props);
this.containerRef = React.createRef();
this.ro = new ResizeObserver(this.handleResize);
this.animationFrameID = NaN;
this.ro = new ResizeObserver(this.onResize);
}

componentDidMount() {
this.setupResizeDebounce();
if (this.containerRef.current) {
this.ro.observe(this.containerRef.current as Element);
}
}

componentDidUpdate({ resizeDebounce }: Readonly<ResizerProps>): void {
if (resizeDebounce !== this.props.resizeDebounce) this.setupResizeDebounce();
}

componentWillUnmount() {
window.cancelAnimationFrame(this.animationFrameID);
this.ro.disconnect();
}

setupResizeDebounce() {
this.onResizeDebounced =
this.props.resizeDebounce > 0 ? debounce(this.onResize, this.props.resizeDebounce) : this.onResize;
}

onResize: ResizeFn = (entries) => {
if (!Array.isArray(entries)) {
return;
Expand All @@ -78,19 +55,8 @@ class Resizer extends React.Component<ResizerProps> {
return;
}
const { width, height } = entries[0].contentRect;
this.animationFrameID = window.requestAnimationFrame(() => {
this.props.updateParentDimensions({ width, height, top: 0, left: 0 });
this.props.onResize?.();
});
};

handleResize = (entries: ResizeObserverEntry[]) => {
if (this.initialResizeComplete) {
this.onResizeDebounced?.(entries);
} else {
this.initialResizeComplete = true;
this.onResize(entries);
}
this.props.updateParentDimensions({ width, height, top: 0, left: 0 });
this.props.onResize?.();
};

render() {
Expand All @@ -99,19 +65,11 @@ class Resizer extends React.Component<ResizerProps> {
}

const mapDispatchToProps = (dispatch: Dispatch): ResizerDispatchProps =>
bindActionCreators(
{
updateParentDimensions,
},
dispatch,
);
bindActionCreators({ updateParentDimensions }, dispatch);

const mapStateToProps = (state: GlobalChartState): ResizerStateProps => {
const { resizeDebounce, onResize } = getSettingsSpecSelector(state);
return {
resizeDebounce: isFiniteNumber(resizeDebounce) ? resizeDebounce : DEFAULT_RESIZE_DEBOUNCE,
onResize,
};
const { onResize } = getSettingsSpecSelector(state);
return { onResize };
};

/** @internal */
Expand Down
4 changes: 0 additions & 4 deletions packages/charts/src/specs/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,6 @@ export const TooltipStickTo = Object.freeze({
/** @public */
export type TooltipStickTo = $Values<typeof TooltipStickTo>;

/** @internal */
export const DEFAULT_RESIZE_DEBOUNCE = 10;

/**
* Default legend config
* @internal
Expand All @@ -154,7 +151,6 @@ export const settingsBuildProps = buildSFProps<SettingsSpec>()(
rendering: 'canvas' as const,
rotation: 0 as const,
animateData: true,
resizeDebounce: DEFAULT_RESIZE_DEBOUNCE,
debug: false,
pointerUpdateTrigger: PointerUpdateTrigger.X,
externalPointerEvents: {
Expand Down
1 change: 1 addition & 0 deletions packages/charts/src/specs/settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,7 @@ export interface SettingsSpec extends Spec, LegendSpec {

/**
* debounce delay used for resizing chart
* @deprecated currently unused
*/
resizeDebounce?: number;

Expand Down
14 changes: 10 additions & 4 deletions packages/charts/src/utils/chart_size.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,7 @@ export interface ChartSizeObject {
export type ChartSize = number | string | ChartSizeArray | ChartSizeObject;

/** @internal */
export function getChartSize(size?: ChartSize): ChartSizeObject {
if (size === undefined) {
return {};
}
export function getChartSize(size?: ChartSize): Required<ChartSizeObject> {
if (Array.isArray(size)) {
return {
width: size[0] === undefined ? '100%' : size[0],
Expand All @@ -40,3 +37,12 @@ export function getChartSize(size?: ChartSize): ChartSizeObject {
height: sameSize,
};
}

/**
* Return the requested size if specified in pixel, null otherwise
* @internal
*/
export function getFixedChartSize(size?: ChartSize): { width: number; height: number } | null {
const { width, height } = getChartSize(size);
return typeof height === 'number' && typeof width === 'number' ? { width, height } : null;
}
82 changes: 0 additions & 82 deletions storybook/stories/test_cases/11_resize_debounce.story.tsx

This file was deleted.

1 change: 0 additions & 1 deletion storybook/stories/test_cases/test_cases.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,4 @@ export { Example as rtlText } from './7_rtl_text.story';
export { Example as testPointsOutsideOfDomain } from './8_test_points_outside_of_domain.story';
export { Example as duplicateLabelsInPartitionLegend } from './9_duplicate_labels_in_partition_legend.story';
export { Example as highlighterZIndex } from './10_highlighter_z_index.story';
export { Example as resizeDebounce } from './11_resize_debounce.story';
export { Example as domainEdges } from './21_domain_edges.story';
8 changes: 2 additions & 6 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -11638,7 +11638,8 @@ eslint-module-utils@^2.8.0:
debug "^3.2.7"

"eslint-plugin-elastic-charts@link:./packages/eslint-plugin-elastic-charts":
version "1.0.0"
version "0.0.0"
uid ""

eslint-plugin-eslint-comments@^3.2.0:
version "3.2.0"
Expand Down Expand Up @@ -20389,11 +20390,6 @@ reselect@^4.0.0:
resolved "https://registry.yarnpkg.com/reselect/-/reselect-4.0.0.tgz#f2529830e5d3d0e021408b246a206ef4ea4437f7"
integrity sha512-qUgANli03jjAyGlnbYVAV5vvnOmJnODyABz51RdBN7M4WaVu8mecZWgyQNkG8Yqe3KRGRt0l4K4B3XVEULC4CA==

resize-observer-polyfill@^1.5.1:
version "1.5.1"
resolved "https://registry.yarnpkg.com/resize-observer-polyfill/-/resize-observer-polyfill-1.5.1.tgz#0e9020dd3d21024458d4ebd27e23e40269810464"
integrity sha512-LwZrotdHOo12nQuZlHEmtuXdqGoOD0OhaxopaNFxWzInpEgaLWoVuAMbTzixuosCx2nEG58ngzW3vxdWoxIgdg==

resolve-cwd@^3.0.0:
version "3.0.0"
resolved "https://registry.yarnpkg.com/resolve-cwd/-/resolve-cwd-3.0.0.tgz#0f0075f1bb2544766cf73ba6a6e2adfebcb13f2d"
Expand Down

0 comments on commit f9c11fc

Please sign in to comment.