-
Notifications
You must be signed in to change notification settings - Fork 134
fix: onRenderChange callback trigger on resize
#2228
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
Changes from all commits
392ebc1
690fdca
da94f2d
97fec15
d05cb15
d7fc01e
3c168c3
414c882
0645223
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,24 +11,25 @@ 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'; | ||
| import { Dimensions } from '../utils/dimensions'; | ||
|
|
||
| interface ResizerStateProps { | ||
| resizeDebounce: number; | ||
| onResize?: ResizeListener; | ||
| } | ||
|
|
||
| interface ResizerDispatchProps { | ||
| updateParentDimensions(dimension: Dimensions): void; | ||
| updateParentDimensions: typeof updateParentDimensions; | ||
| } | ||
|
|
||
| type ResizerProps = ResizerStateProps & ResizerDispatchProps; | ||
|
|
||
| const DEFAULT_RESIZE_DEBOUNCE = 200; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was never used cuz |
||
| type ResizeFn = (entries: ResizeObserverEntry[]) => void; | ||
|
|
||
| class Resizer extends React.Component<ResizerProps> { | ||
| private initialResizeComplete = false; | ||
|
|
@@ -39,10 +40,7 @@ class Resizer extends React.Component<ResizerProps> { | |
|
|
||
| private animationFrameID: number; | ||
|
|
||
| private onResizeDebounced?: DebouncedFunction< | ||
| [entries: ResizeObserverEntry[]], | ||
| (entries: ResizeObserverEntry[]) => void | ||
| >; | ||
| private onResizeDebounced?: ResizeFn | DebouncedFunction<Parameters<ResizeFn>, ResizeFn>; | ||
|
|
||
| constructor(props: ResizerProps) { | ||
| super(props); | ||
|
|
@@ -52,18 +50,27 @@ class Resizer extends React.Component<ResizerProps> { | |
| } | ||
|
|
||
| componentDidMount() { | ||
| this.onResizeDebounced = debounce(this.onResize, this.props.resizeDebounce); | ||
| 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(); | ||
| } | ||
|
Comment on lines
+59
to
+61
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If Even setting the value directly on the settings with a storybook knob was too async to update the value and thus never using the initial knob value. |
||
|
|
||
| componentWillUnmount() { | ||
| window.cancelAnimationFrame(this.animationFrameID); | ||
| this.ro.disconnect(); | ||
| } | ||
|
|
||
| onResize = (entries: ResizeObserverEntry[]) => { | ||
| setupResizeDebounce() { | ||
| this.onResizeDebounced = | ||
| this.props.resizeDebounce > 0 ? debounce(this.onResize, this.props.resizeDebounce) : this.onResize; | ||
| } | ||
|
|
||
| onResize: ResizeFn = (entries) => { | ||
| if (!Array.isArray(entries)) { | ||
| return; | ||
| } | ||
|
|
@@ -73,6 +80,7 @@ class Resizer extends React.Component<ResizerProps> { | |
| const { width, height } = entries[0].contentRect; | ||
| this.animationFrameID = window.requestAnimationFrame(() => { | ||
| this.props.updateParentDimensions({ width, height, top: 0, left: 0 }); | ||
| this.props.onResize?.(); | ||
| }); | ||
| }; | ||
|
|
||
|
|
@@ -99,9 +107,10 @@ const mapDispatchToProps = (dispatch: Dispatch): ResizerDispatchProps => | |
| ); | ||
|
|
||
| const mapStateToProps = (state: GlobalChartState): ResizerStateProps => { | ||
| const { resizeDebounce } = getSettingsSpecSelector(state); | ||
| const { resizeDebounce, onResize } = getSettingsSpecSelector(state); | ||
| return { | ||
| resizeDebounce: isFiniteNumber(resizeDebounce) ? resizeDebounce : DEFAULT_RESIZE_DEBOUNCE, | ||
| onResize, | ||
| }; | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| /* | ||
| * 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 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 or the Server | ||
| * Side Public License, v 1. | ||
| */ | ||
|
|
||
| import { action } from '@storybook/addon-actions'; | ||
| import { number } from '@storybook/addon-knobs'; | ||
| import moment from 'moment'; | ||
| import React from 'react'; | ||
|
|
||
| import { Axis, Chart, BarSeries, Position, ScaleType, Settings } from '@elastic/charts'; | ||
| import { getRandomNumberGenerator } from '@elastic/charts/src/mocks/utils'; | ||
|
|
||
| import { ChartsStory } from '../../types'; | ||
| import { useBaseTheme } from '../../use_base_theme'; | ||
|
|
||
| const rng = getRandomNumberGenerator(); | ||
|
|
||
| const data: { t: number; values: { v: number; cat: string }[] }[] = []; | ||
| const end = moment(1699037055867); | ||
| const days = 3; | ||
| const maxCardinality = 100; | ||
| const start = end.clone().subtract(days, 'days'); | ||
| const hours = 6; | ||
| while (start.isBefore(end)) { | ||
| const values = Array.from({ length: maxCardinality }, (_, i) => ({ | ||
| v: rng(0, 100), | ||
| cat: `Category ${i + 1}`, | ||
| })); | ||
| data.push({ t: start.add(hours, 'hours').valueOf(), values }); | ||
| } | ||
|
|
||
| export const Example: ChartsStory = (_, { title, description }) => { | ||
| const resizeDebounce = number('resizeDebounce (ms)', 10, { min: 0, step: 20 }); | ||
| const cardinality = number('cardinality', 100, { min: 1, max: maxCardinality }); | ||
| return ( | ||
| <div | ||
| style={{ | ||
| resize: 'both', | ||
| padding: '0px', | ||
| overflow: 'auto', | ||
| height: '100%', | ||
| width: '100%', | ||
| maxWidth: '100%', | ||
| maxHeight: '100vh', | ||
| }} | ||
| > | ||
| <Chart title={title} description={description}> | ||
| <Settings | ||
| showLegend | ||
| resizeDebounce={resizeDebounce} | ||
| onResize={action('onResize')} | ||
| onRenderChange={action('onRenderChange')} | ||
| baseTheme={useBaseTheme()} | ||
| /> | ||
| <Axis | ||
| id="bottom" | ||
| position={Position.Bottom} | ||
| style={{ | ||
| tickLine: { visible: true, size: 0.0001, padding: 4 }, | ||
| tickLabel: { | ||
| alignment: { horizontal: Position.Left, vertical: Position.Bottom }, | ||
| padding: 0, | ||
| offset: { x: 0, y: 0 }, | ||
| }, | ||
| }} | ||
| timeAxisLayerCount={3} | ||
| /> | ||
| <Axis id="left" position={Position.Left} /> | ||
|
|
||
| <BarSeries | ||
| id="Sensor 1" | ||
| enableHistogramMode | ||
| xScaleType={ScaleType.Time} | ||
| yScaleType={ScaleType.Linear} | ||
| xAccessor="t" | ||
| yAccessors={['v']} | ||
| splitSeriesAccessors={['cat']} | ||
| stackAccessors={['yes']} | ||
| data={data.flatMap(({ t, values }) => values.slice(0, cardinality).map(({ v, cat }) => ({ t, v, cat })))} | ||
| /> | ||
| </Chart> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| Example.parameters = { | ||
| markdown: `The \`resizeDebounce\` option on the \`Settings\` spec provides control over the eagerness of the chart to re-render upon resize. A value of \`0\` will remove the debounce altogether. | ||
| You can play with the cardinality and debounce time to see how the debouncing affects the chart render timing`, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could consider removing this deboucer altogether as it was an original implementation that may not be needed or at the very least default to
0(aka disabled).