From cd72025f2e1e584a94291661c40715a3c69433e6 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Fri, 26 Jul 2024 18:02:48 -0500 Subject: [PATCH 1/2] feat: Allow ref callback for Chart and ChartPanel --- packages/chart/src/Chart.tsx | 11 +++-- .../src/panels/ChartPanel.tsx | 7 +++- packages/utils/src/index.ts | 1 + packages/utils/src/mergeRefs.test.ts | 41 +++++++++++++++++++ packages/utils/src/mergeRefs.ts | 31 ++++++++++++++ 5 files changed, 85 insertions(+), 6 deletions(-) create mode 100644 packages/utils/src/mergeRefs.test.ts create mode 100644 packages/utils/src/mergeRefs.ts diff --git a/packages/chart/src/Chart.tsx b/packages/chart/src/Chart.tsx index 88c983784..952154bb6 100644 --- a/packages/chart/src/Chart.tsx +++ b/packages/chart/src/Chart.tsx @@ -28,7 +28,7 @@ import { ModeBarButtonAny, } from 'plotly.js'; import type { PlotParams } from 'react-plotly.js'; -import { bindAllMethods } from '@deephaven/utils'; +import { bindAllMethods, mergeRefs } from '@deephaven/utils'; import createPlotlyComponent from './plotly/createPlotlyComponent'; import Plotly from './plotly/Plotly'; import ChartModel from './ChartModel'; @@ -57,7 +57,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 +156,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 +239,8 @@ class Chart extends Component { plotWrapper: RefObject; + plotWrapperMerged: React.RefCallback; + columnFormats?: FormattingRule[]; dateTimeFormatterOptions?: DateTimeColumnFormatterOptions; @@ -713,7 +716,7 @@ class Chart extends Component { const isPlotShown = data != null; return ( -
+
{isPlotShown && ( 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/utils/src/index.ts b/packages/utils/src/index.ts index dc0afb0ad..df3b13a01 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -27,3 +27,4 @@ export { default as ValidationError } from './ValidationError'; export { default as TestUtils } from './TestUtils'; export * from './TestUtils'; export * from './UIConstants'; +export * from './mergeRefs'; diff --git a/packages/utils/src/mergeRefs.test.ts b/packages/utils/src/mergeRefs.test.ts new file mode 100644 index 000000000..bfb185780 --- /dev/null +++ b/packages/utils/src/mergeRefs.test.ts @@ -0,0 +1,41 @@ +import React from 'react'; +import { mergeRefs } from './mergeRefs'; + +describe('mergeRefs', () => { + test('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); + }); + + test('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); + }); + + test('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); + }); +}); diff --git a/packages/utils/src/mergeRefs.ts b/packages/utils/src/mergeRefs.ts new file mode 100644 index 000000000..d66a0e80c --- /dev/null +++ b/packages/utils/src/mergeRefs.ts @@ -0,0 +1,31 @@ +import type React 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: Array< + React.MutableRefObject | React.LegacyRef | null | undefined + > +): React.RefCallback { + return value => { + refs.forEach(ref => { + if (ref != null) { + if (typeof ref === 'function') { + ref(value); + } 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 = value; + } + } + }); + }; +} + +export default mergeRefs; From 8b3ca50ca26fb2d0b4ff56ef87baa13d7da56424 Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Tue, 30 Jul 2024 12:04:53 -0500 Subject: [PATCH 2/2] Address review comments --- packages/chart/src/Chart.tsx | 3 +- .../src/spectrum/comboBox/ComboBox.tsx | 4 +- .../components/src/spectrum/picker/Picker.tsx | 4 +- .../src/spectrum/picker/useMultiRef.ts | 22 -------- packages/react-hooks/src/index.ts | 1 + .../src/useMergeRef.test.ts} | 52 ++++++++++++++++--- packages/react-hooks/src/useMergeRef.ts | 43 +++++++++++++++ packages/utils/src/index.ts | 1 - packages/utils/src/mergeRefs.test.ts | 41 --------------- packages/utils/src/mergeRefs.ts | 31 ----------- 10 files changed, 96 insertions(+), 106 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 delete mode 100644 packages/utils/src/mergeRefs.test.ts delete mode 100644 packages/utils/src/mergeRefs.ts diff --git a/packages/chart/src/Chart.tsx b/packages/chart/src/Chart.tsx index 952154bb6..2b389233e 100644 --- a/packages/chart/src/Chart.tsx +++ b/packages/chart/src/Chart.tsx @@ -28,7 +28,8 @@ import { ModeBarButtonAny, } from 'plotly.js'; import type { PlotParams } from 'react-plotly.js'; -import { bindAllMethods, mergeRefs } from '@deephaven/utils'; +import { mergeRefs } from '@deephaven/react-hooks'; +import { bindAllMethods } from '@deephaven/utils'; import createPlotlyComponent from './plotly/createPlotlyComponent'; import Plotly from './plotly/Plotly'; import ChartModel from './ChartModel'; diff --git a/packages/components/src/spectrum/comboBox/ComboBox.tsx b/packages/components/src/spectrum/comboBox/ComboBox.tsx index 80809b4fd..4a689acd1 100644 --- a/packages/components/src/spectrum/comboBox/ComboBox.tsx +++ b/packages/components/src/spectrum/comboBox/ComboBox.tsx @@ -5,9 +5,9 @@ import { } from '@adobe/react-spectrum'; import type { DOMRef } from '@react-types/shared'; import cl from 'classnames'; +import { useMergeRef } from '@deephaven/react-hooks'; import type { NormalizedItem } from '../utils'; import { PickerPropsT, usePickerProps } from '../picker'; -import useMultiRef from '../picker/useMultiRef'; export type ComboBoxProps = PickerPropsT>; @@ -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/react-hooks/src/index.ts b/packages/react-hooks/src/index.ts index 83145b70d..de94dbe05 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 7237f3c2d..70a01e4d0 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 000000000..3e9829397 --- /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); +} diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index df3b13a01..dc0afb0ad 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -27,4 +27,3 @@ export { default as ValidationError } from './ValidationError'; export { default as TestUtils } from './TestUtils'; export * from './TestUtils'; export * from './UIConstants'; -export * from './mergeRefs'; diff --git a/packages/utils/src/mergeRefs.test.ts b/packages/utils/src/mergeRefs.test.ts deleted file mode 100644 index bfb185780..000000000 --- a/packages/utils/src/mergeRefs.test.ts +++ /dev/null @@ -1,41 +0,0 @@ -import React from 'react'; -import { mergeRefs } from './mergeRefs'; - -describe('mergeRefs', () => { - test('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); - }); - - test('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); - }); - - test('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); - }); -}); diff --git a/packages/utils/src/mergeRefs.ts b/packages/utils/src/mergeRefs.ts deleted file mode 100644 index d66a0e80c..000000000 --- a/packages/utils/src/mergeRefs.ts +++ /dev/null @@ -1,31 +0,0 @@ -import type React 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: Array< - React.MutableRefObject | React.LegacyRef | null | undefined - > -): React.RefCallback { - return value => { - refs.forEach(ref => { - if (ref != null) { - if (typeof ref === 'function') { - ref(value); - } 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 = value; - } - } - }); - }; -} - -export default mergeRefs;