From 56d1fa9ba00d319794d686365be245c757ad2178 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Tue, 30 Jul 2024 15:12:56 -0500 Subject: [PATCH] feat: Allow ref callback for Chart and ChartPanel (#2174) Needed for https://github.com/deephaven/deephaven-plugins/issues/657 because when using ChartPanel `containerRef` is undefined on first render due to the loading logic. This means we need to allow a ref callback so we can use a callback to set state in the plugin and add event listeners to the container when it actually exists --- packages/chart/src/Chart.tsx | 10 ++-- .../src/spectrum/comboBox/ComboBox.tsx | 4 +- .../components/src/spectrum/picker/Picker.tsx | 4 +- .../src/spectrum/picker/useMultiRef.ts | 22 -------- .../src/panels/ChartPanel.tsx | 7 ++- packages/react-hooks/src/index.ts | 1 + .../src/useMergeRef.test.ts} | 52 ++++++++++++++++--- packages/react-hooks/src/useMergeRef.ts | 43 +++++++++++++++ 8 files changed, 106 insertions(+), 37 deletions(-) delete mode 100644 packages/components/src/spectrum/picker/useMultiRef.ts rename packages/{components/src/spectrum/picker/useMultiRef.test.ts => react-hooks/src/useMergeRef.test.ts} (53%) create mode 100644 packages/react-hooks/src/useMergeRef.ts diff --git a/packages/chart/src/Chart.tsx b/packages/chart/src/Chart.tsx index 88c983784a..2b389233ed 100644 --- a/packages/chart/src/Chart.tsx +++ b/packages/chart/src/Chart.tsx @@ -28,6 +28,7 @@ import { ModeBarButtonAny, } from 'plotly.js'; import type { PlotParams } from 'react-plotly.js'; +import { mergeRefs } from '@deephaven/react-hooks'; import { bindAllMethods } from '@deephaven/utils'; import createPlotlyComponent from './plotly/createPlotlyComponent'; import Plotly from './plotly/Plotly'; @@ -57,7 +58,7 @@ interface ChartProps { isActive: boolean; Plotly: typeof Plotly; - containerRef?: React.RefObject; + containerRef?: React.Ref; onDisconnect: () => void; onReconnect: () => void; onUpdate: (obj: { isLoading: boolean }) => void; @@ -156,7 +157,8 @@ class Chart extends Component { this.PlotComponent = createPlotlyComponent(props.Plotly); this.plot = React.createRef(); - this.plotWrapper = props.containerRef ?? React.createRef(); + this.plotWrapper = React.createRef(); + this.plotWrapperMerged = mergeRefs(this.plotWrapper, props.containerRef); this.columnFormats = []; this.dateTimeFormatterOptions = {}; this.decimalFormatOptions = {}; @@ -238,6 +240,8 @@ class Chart extends Component { plotWrapper: RefObject; + plotWrapperMerged: React.RefCallback; + columnFormats?: FormattingRule[]; dateTimeFormatterOptions?: DateTimeColumnFormatterOptions; @@ -713,7 +717,7 @@ class Chart extends Component { const isPlotShown = data != null; return ( -
+
{isPlotShown && ( >; @@ -22,7 +22,7 @@ export const ComboBox = React.forwardRef(function ComboBox( ref: scrollRef, ...comboBoxProps } = usePickerProps(props); - const pickerRef = useMultiRef(ref, scrollRef); + const pickerRef = useMergeRef(ref, scrollRef); return ( (...refs: readonly Ref[]): RefCallback { - return useCallback(newRef => { - refs.forEach(ref => { - if (typeof ref === 'function') { - ref(newRef); - } else if (ref != null) { - // eslint-disable-next-line no-param-reassign - (ref as MutableRefObject).current = newRef; - } - }); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, refs); -} - -export default useMultiRef; diff --git a/packages/dashboard-core-plugins/src/panels/ChartPanel.tsx b/packages/dashboard-core-plugins/src/panels/ChartPanel.tsx index e100ec5ef8..717d2d2a26 100644 --- a/packages/dashboard-core-plugins/src/panels/ChartPanel.tsx +++ b/packages/dashboard-core-plugins/src/panels/ChartPanel.tsx @@ -124,8 +124,11 @@ interface OwnProps extends DashboardPanelProps { makeModel: () => Promise; localDashboardId: string; Plotly?: typeof PlotlyType; - /** The plot container div */ - containerRef?: RefObject; + /** + * The plot container div. + * The ref will be undefined on initial render if the chart needs to be loaded. + */ + containerRef?: React.Ref; panelState?: GLChartPanelState; } diff --git a/packages/react-hooks/src/index.ts b/packages/react-hooks/src/index.ts index 83145b70db..de94dbe05e 100644 --- a/packages/react-hooks/src/index.ts +++ b/packages/react-hooks/src/index.ts @@ -34,3 +34,4 @@ export * from './useSetAttributesCallback'; export * from './useSpectrumDisableSpellcheckRef'; export * from './useWindowedListData'; export * from './useResizeObserver'; +export * from './useMergeRef'; diff --git a/packages/components/src/spectrum/picker/useMultiRef.test.ts b/packages/react-hooks/src/useMergeRef.test.ts similarity index 53% rename from packages/components/src/spectrum/picker/useMultiRef.test.ts rename to packages/react-hooks/src/useMergeRef.test.ts index 7237f3c2da..70a01e4d09 100644 --- a/packages/components/src/spectrum/picker/useMultiRef.test.ts +++ b/packages/react-hooks/src/useMergeRef.test.ts @@ -1,12 +1,52 @@ +import React from 'react'; import { renderHook } from '@testing-library/react-hooks'; -import useMultiRef from './useMultiRef'; +import { useMergeRef, mergeRefs } from './useMergeRef'; -describe('useMultiRef', () => { +describe('mergeRefs', () => { + it('merges ref objects', () => { + const refA = React.createRef(); + const refB = React.createRef(); + const mergedRef = mergeRefs(refA, refB); + + const refValue = {}; + mergedRef(refValue); + expect(refA.current).toBe(refValue); + expect(refB.current).toBe(refValue); + }); + + it('merges ref callbacks', () => { + const refA: React.RefCallback = jest.fn(); + const refB: React.RefCallback = jest.fn(); + const mergedRef = mergeRefs(refA, refB); + + const refValue = {}; + mergedRef(refValue); + expect(refA).toHaveBeenCalledWith(refValue); + expect(refB).toHaveBeenCalledWith(refValue); + }); + + it('ignores null/undefined refs', () => { + const refA = React.createRef(); + const refB: React.RefCallback = jest.fn(); + const refC = null; + const refD = undefined; + const mergedRef = mergeRefs(refA, refB, refC, refD); + + const refValue = {}; + mergedRef(refValue); + expect(refA.current).toBe(refValue); + expect(refB).toHaveBeenCalledWith(refValue); + expect(refC).toBe(null); + expect(refD).toBe(undefined); + }); +}); + +describe('useMergeRef', () => { it('should assign the ref to all refs passed in', () => { const ref1 = jest.fn(); const ref2 = jest.fn(); const ref3 = jest.fn(); - const { result } = renderHook(() => useMultiRef(ref1, ref2, ref3)); + const { result } = renderHook(() => useMergeRef(ref1, ref2, ref3)); const multiRef = result.current; const element = document.createElement('div'); multiRef(element); @@ -19,7 +59,7 @@ describe('useMultiRef', () => { const ref1 = jest.fn(); const ref2 = jest.fn(); const ref3 = jest.fn(); - const { result } = renderHook(() => useMultiRef(ref1, ref2, ref3)); + const { result } = renderHook(() => useMergeRef(ref1, ref2, ref3)); const multiRef = result.current; multiRef(null); expect(ref1).toHaveBeenCalledWith(null); @@ -32,7 +72,7 @@ describe('useMultiRef', () => { const ref2 = { current: null }; const ref3 = { current: null }; const { result } = renderHook(() => - useMultiRef(ref1, ref2, ref3) + useMergeRef(ref1, ref2, ref3) ); const multiRef = result.current; const element = document.createElement('div'); @@ -47,7 +87,7 @@ describe('useMultiRef', () => { const ref2 = { current: null }; const ref3 = jest.fn(); const { result } = renderHook(() => - useMultiRef(ref1, ref2, ref3) + useMergeRef(ref1, ref2, ref3) ); const multiRef = result.current; const element = document.createElement('div'); diff --git a/packages/react-hooks/src/useMergeRef.ts b/packages/react-hooks/src/useMergeRef.ts new file mode 100644 index 0000000000..3e98293978 --- /dev/null +++ b/packages/react-hooks/src/useMergeRef.ts @@ -0,0 +1,43 @@ +import { + type LegacyRef, + type MutableRefObject, + type Ref, + type RefCallback, + useMemo, +} from 'react'; + +/** + * Merge multiple react refs into a single ref callback. + * This can be used to merge callback and object refs into a single ref. + * Merged callback refs will be called while object refs will have their current property set. + * @param refs The refs to merge + * @returns A ref callback that will set the value on all refs + */ +export function mergeRefs( + ...refs: readonly (MutableRefObject | LegacyRef | null | undefined)[] +): RefCallback { + return newRef => { + refs.forEach(ref => { + if (ref != null) { + if (typeof ref === 'function') { + ref(newRef); + } else { + // React marks RefObject as readonly, but it's just to indicate React manages it + // We can still write to its current value + // eslint-disable-next-line no-param-reassign + (ref as React.MutableRefObject).current = newRef; + } + } + }); + }; +} + +/** + * Merges multiple refs into one ref that can be assigned to the component. + * In turn all the refs passed in will be assigned when the ref returned is assigned. + * @param refs Array of refs to assign + */ +export function useMergeRef(...refs: readonly Ref[]): RefCallback { + // eslint-disable-next-line react-hooks/exhaustive-deps + return useMemo(() => mergeRefs(...refs), refs); +}