From 4ec92658a7be443fc8b409a6beaf7c2ecf9e1a5e Mon Sep 17 00:00:00 2001 From: Kenan Date: Thu, 16 May 2024 14:10:37 +0100 Subject: [PATCH 1/6] refactor victory-portal to remove dependency on react-dom --- packages/victory-core/src/index.ts | 1 - .../victory-container/victory-container.tsx | 34 ++++----- .../src/victory-portal/portal-context.ts | 15 ---- .../src/victory-portal/portal-context.tsx | 69 +++++++++++++++++++ .../src/victory-portal/portal-outlet.tsx | 25 +++++++ .../src/victory-portal/victory-portal.tsx | 57 ++++++++------- stories/victory-portal.stories.tsx | 57 +++++++++++++++ 7 files changed, 197 insertions(+), 61 deletions(-) delete mode 100644 packages/victory-core/src/victory-portal/portal-context.ts create mode 100644 packages/victory-core/src/victory-portal/portal-context.tsx create mode 100644 packages/victory-core/src/victory-portal/portal-outlet.tsx create mode 100644 stories/victory-portal.stories.tsx diff --git a/packages/victory-core/src/index.ts b/packages/victory-core/src/index.ts index f8980cddd..ccefa312a 100644 --- a/packages/victory-core/src/index.ts +++ b/packages/victory-core/src/index.ts @@ -9,7 +9,6 @@ export * from "./victory-clip-container/victory-clip-container"; export * from "./victory-theme/types"; export * from "./victory-theme/victory-theme"; export * from "./victory-portal/portal"; -export * from "./victory-portal/portal-context"; export * from "./victory-portal/victory-portal"; export * from "./victory-primitives"; export { Border as Box } from "./victory-primitives"; diff --git a/packages/victory-core/src/victory-container/victory-container.tsx b/packages/victory-core/src/victory-container/victory-container.tsx index 0e53148fe..6419c9ece 100644 --- a/packages/victory-core/src/victory-container/victory-container.tsx +++ b/packages/victory-core/src/victory-container/victory-container.tsx @@ -1,12 +1,13 @@ import React, { useRef } from "react"; import { uniqueId } from "lodash"; import { Portal } from "../victory-portal/portal"; -import { PortalContext } from "../victory-portal/portal-context"; import * as UserProps from "../victory-util/user-props"; import { OriginType } from "../victory-label/victory-label"; import { D3Scale } from "../types/prop-types"; import { VictoryThemeDefinition } from "../victory-theme/types"; import { mergeRefs } from "../victory-util"; +import { PortalOutlet } from "../victory-portal/portal-outlet"; +import { PortalProvider } from "../victory-portal/portal-context"; export interface VictoryContainerProps { "aria-describedby"?: string; @@ -58,10 +59,6 @@ export function useVictoryContainer( const localContainerRef = useRef(null); - const [portalElement, setPortalElement] = React.useState(); - - const portalRef = (element: SVGSVGElement) => setPortalElement(element); - // Generated ID stored in ref because it needs to persist across renders const generatedId = useRef(uniqueId("victory-container-")); const containerId = props.containerId ?? generatedId.current; @@ -103,8 +100,6 @@ export function useVictoryContainer( ariaLabelledBy, ariaDescribedBy, userProps, - portalRef, - portalElement, localContainerRef, }; } @@ -135,8 +130,6 @@ export const VictoryContainer = (initialProps: VictoryContainerProps) => { userProps, titleId, descId, - portalElement, - portalRef, containerRef, localContainerRef, } = useVictoryContainer(initialProps); @@ -156,7 +149,7 @@ export const VictoryContainer = (initialProps: VictoryContainerProps) => { }, []); return ( - +
{ left: 0, }} > - {React.cloneElement(portalComponent, { - width, - height, - viewBox, - preserveAspectRatio, - style: { ...dimensions, overflow: "visible" }, - ref: portalRef, - })} +
- + ); }; diff --git a/packages/victory-core/src/victory-portal/portal-context.ts b/packages/victory-core/src/victory-portal/portal-context.ts deleted file mode 100644 index 3551dcaf4..000000000 --- a/packages/victory-core/src/victory-portal/portal-context.ts +++ /dev/null @@ -1,15 +0,0 @@ -import React from "react"; - -export interface PortalContextValue { - portalElement: SVGSVGElement | undefined; -} - -/** - * The React context object consumers may use to access the context of the - * portal. - */ -export const PortalContext = React.createContext< - PortalContextValue | undefined ->(undefined); - -PortalContext.displayName = "PortalContext"; diff --git a/packages/victory-core/src/victory-portal/portal-context.tsx b/packages/victory-core/src/victory-portal/portal-context.tsx new file mode 100644 index 000000000..10bcd95f7 --- /dev/null +++ b/packages/victory-core/src/victory-portal/portal-context.tsx @@ -0,0 +1,69 @@ +import React from "react"; + +export interface PortalContextValue { + addChild: (id: string, node: React.ReactElement) => void; + removeChild: (id: string) => void; + children: Map; +} + +export const PortalContext = React.createContext< + PortalContextValue | undefined +>(undefined); +PortalContext.displayName = "PortalContext"; + +export const usePortalContext = () => { + const context = React.useContext(PortalContext); + + if (!context) { + throw new Error( + "`usePortalContext` must be used within a ``", + ); + } + return context; +}; + +export interface PortalProviderProps { + children?: React.ReactNode; +} + +export const PortalProvider = ({ children }: PortalProviderProps) => { + const [portalChildren, setPortalChildren] = React.useState< + Map + >(new Map()); + const addChild = React.useCallback( + (id: string, element: React.ReactElement) => { + setPortalChildren((prevChildren) => { + const nextChildren = new Map(prevChildren); + nextChildren.set(id, element); + return nextChildren; + }); + }, + [setPortalChildren], + ); + + const removeChild = React.useCallback( + (id: string) => { + setPortalChildren((prevChildren) => { + const nextChildren = new Map(prevChildren); + nextChildren.delete(id); + return nextChildren; + }); + }, + [setPortalChildren], + ); + + const contextValue: PortalContextValue = React.useMemo( + () => ({ + addChild, + removeChild, + children: portalChildren, + }), + [addChild, removeChild, portalChildren], + ); + + return ( + + {children} + + ); +}; diff --git a/packages/victory-core/src/victory-portal/portal-outlet.tsx b/packages/victory-core/src/victory-portal/portal-outlet.tsx new file mode 100644 index 000000000..be8b673b9 --- /dev/null +++ b/packages/victory-core/src/victory-portal/portal-outlet.tsx @@ -0,0 +1,25 @@ +import React from "react"; +import { usePortalContext } from "./portal-context"; + +export interface PortalOutletProps { + as: React.ReactElement; + width?: number; + height?: number; + viewBox?: string; + preserveAspectRatio?: string; + style?: React.CSSProperties; + children?: (children: React.ReactElement[]) => React.ReactNode | undefined; +} + +export const PortalOutlet = ({ + as: portalComponent, + ...props +}: PortalOutletProps) => { + const portalContext = usePortalContext(); + + const children = Array.from(portalContext.children.entries()).map( + ([key, node]) => (!node.key ? React.cloneElement(node, { key }) : node), + ); + + return React.cloneElement(portalComponent, props, children); +}; diff --git a/packages/victory-core/src/victory-portal/victory-portal.tsx b/packages/victory-core/src/victory-portal/victory-portal.tsx index b6f57ae5d..e4533c4df 100644 --- a/packages/victory-core/src/victory-portal/victory-portal.tsx +++ b/packages/victory-core/src/victory-portal/victory-portal.tsx @@ -2,21 +2,21 @@ import React from "react"; import { defaults } from "lodash"; import * as Log from "../victory-util/log"; import * as Helpers from "../victory-util/helpers"; -import { PortalContext } from "./portal-context"; -import { createPortal } from "react-dom"; +import { usePortalContext } from "./portal-context"; export interface VictoryPortalProps { - children?: React.ReactElement; + children: React.ReactElement; groupComponent?: React.ReactElement; } -const defaultProps: VictoryPortalProps = { +const defaultProps: Partial = { groupComponent: , }; export const VictoryPortal = (initialProps: VictoryPortalProps) => { const props = { ...defaultProps, ...initialProps }; - const portalContext = React.useContext(PortalContext); + const id = React.useId(); + const portalContext = usePortalContext(); if (!portalContext) { const msg = @@ -25,25 +25,30 @@ export const VictoryPortal = (initialProps: VictoryPortalProps) => { Log.warn(msg); } - const children = ( - Array.isArray(props.children) ? props.children[0] : props.children - ) as React.ReactElement; - const { groupComponent } = props; - const childProps = (children && children.props) || {}; - const standardProps = childProps.groupComponent - ? { groupComponent, standalone: false } - : {}; - const newProps = defaults( - standardProps, - childProps, - Helpers.omit(props, ["children", "groupComponent"]), - ); - const child = children && React.cloneElement(children, newProps); - - // If there is no portal context, render the child in place - return portalContext?.portalElement - ? createPortal(child, portalContext.portalElement) - : child; -}; + React.useEffect(() => { + const children = Array.isArray(props.children) + ? props.children[0] + : props.children; + const { groupComponent } = props; + const childProps = (children && children.props) || {}; + const standardProps = childProps.groupComponent + ? { groupComponent, standalone: false } + : {}; + const newProps = defaults( + standardProps, + childProps, + Helpers.omit(props, ["children", "groupComponent"]), + ); + const child = children && React.cloneElement(children, newProps); + + portalContext.addChild(id, child); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [props.children]); -VictoryPortal.role = "portal"; + React.useEffect(() => { + return () => portalContext.removeChild(id); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + return null; +}; diff --git a/stories/victory-portal.stories.tsx b/stories/victory-portal.stories.tsx new file mode 100644 index 000000000..a763bfac1 --- /dev/null +++ b/stories/victory-portal.stories.tsx @@ -0,0 +1,57 @@ +import React from "react"; +import { VictoryChart } from "../packages/victory-chart"; +import { VictoryBar } from "../packages/victory-bar"; +import { VictoryGroup } from "../packages/victory-group"; +import { VictoryLabel, VictoryPortal } from "../packages/victory-core"; +import { Meta } from "@storybook/react"; +import { storyContainer } from "./decorators"; + +const meta: Meta = { + title: "Victory Charts/SVG Container/VictoryPortal", + component: VictoryPortal, + tags: ["autodocs"], + decorators: [storyContainer], +}; + +export default meta; + +export const Default = () => { + return ( +
+ + + + + + } + data={[ + { x: 1, y: 1 }, + { x: 2, y: 2 }, + { x: 3, y: 5 }, + ]} + /> + + + + +
+ ); +}; From ab15c957eb900d6b73524e6e7c171b8e7e6fc813 Mon Sep 17 00:00:00 2001 From: Kenan Date: Thu, 16 May 2024 14:42:54 +0100 Subject: [PATCH 2/6] refactor victory-native portal usage --- packages/victory-core/src/index.ts | 2 ++ .../src/components/victory-container.tsx | 13 ++++++------- packages/victory-tooltip/src/victory-tooltip.tsx | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/victory-core/src/index.ts b/packages/victory-core/src/index.ts index ccefa312a..d9d4666b7 100644 --- a/packages/victory-core/src/index.ts +++ b/packages/victory-core/src/index.ts @@ -10,6 +10,8 @@ export * from "./victory-theme/types"; export * from "./victory-theme/victory-theme"; export * from "./victory-portal/portal"; export * from "./victory-portal/victory-portal"; +export * from "./victory-portal/portal-context"; +export * from "./victory-portal/portal-outlet"; export * from "./victory-primitives"; export { Border as Box } from "./victory-primitives"; export type { BorderProps as BoxProps } from "./victory-primitives"; diff --git a/packages/victory-native/src/components/victory-container.tsx b/packages/victory-native/src/components/victory-container.tsx index d63763013..a349afc06 100644 --- a/packages/victory-native/src/components/victory-container.tsx +++ b/packages/victory-native/src/components/victory-container.tsx @@ -7,7 +7,8 @@ import { VictoryEventHandler, mergeRefs, useVictoryContainer, - PortalContext, + PortalProvider, + PortalOutlet, } from "victory-core/es"; import NativeHelpers from "../helpers/native-helpers"; import { Portal } from "./victory-portal/portal"; @@ -41,8 +42,6 @@ export const VictoryContainer = (initialProps: VictoryContainerNativeProps) => { viewBox, preserveAspectRatio, userProps, - portalRef, - portalElement, containerRef, events, onTouchStart, @@ -124,7 +123,7 @@ export const VictoryContainer = (initialProps: VictoryContainerNativeProps) => { const baseStyle = NativeHelpers.getStyle(style, ["width", "height"]); return ( - + { }} pointerEvents="box-none" > - } width={width} height={height} viewBox={viewBox} style={{ ...dimensions, overflow: "visible" }} - ref={portalRef} /> - + ); }; diff --git a/packages/victory-tooltip/src/victory-tooltip.tsx b/packages/victory-tooltip/src/victory-tooltip.tsx index b79e401e3..c851cda71 100644 --- a/packages/victory-tooltip/src/victory-tooltip.tsx +++ b/packages/victory-tooltip/src/victory-tooltip.tsx @@ -600,7 +600,7 @@ export class VictoryTooltip extends React.Component { const active = Helpers.evaluateProp(props.active, props); const { renderInPortal } = props; if (!active) { - return renderInPortal ? : null; + return null; } const evaluatedProps = this.getEvaluatedProps(props); const { flyoutComponent, labelComponent, groupComponent } = evaluatedProps; From ff10d5f55dad816497c16a3c8b34aec70857a1f8 Mon Sep 17 00:00:00 2001 From: Kenan Date: Thu, 16 May 2024 15:15:16 +0100 Subject: [PATCH 3/6] fix some tests --- packages/victory-core/src/exports.test.ts | 8 ++++++++ .../victory-core/src/victory-portal/victory-portal.tsx | 2 ++ packages/victory/src/victory.test.ts | 3 +++ 3 files changed, 13 insertions(+) diff --git a/packages/victory-core/src/exports.test.ts b/packages/victory-core/src/exports.test.ts index e4f2f9065..4937f167c 100644 --- a/packages/victory-core/src/exports.test.ts +++ b/packages/victory-core/src/exports.test.ts @@ -66,6 +66,10 @@ import { Portal, PortalContext, PortalContextValue, + PortalOutlet, + PortalOutletProps, + PortalProvider, + PortalProviderProps, PortalProps, RangePropType, RangeTuple, @@ -137,6 +141,7 @@ import { Wrapper, addEvents, mergeRefs, + usePortalContext, } from "./index"; import { pick } from "lodash"; @@ -173,6 +178,8 @@ describe("victory-core", () => { "PointPathHelpers", "Portal", "PortalContext", + "PortalOutlet", + "PortalProvider", "Rect", "Scale", "Selection", @@ -196,6 +203,7 @@ describe("victory-core", () => { "Wrapper", "addEvents", "mergeRefs", + "usePortalContext", "useVictoryContainer", ] `); diff --git a/packages/victory-core/src/victory-portal/victory-portal.tsx b/packages/victory-core/src/victory-portal/victory-portal.tsx index e4533c4df..95bb66fd5 100644 --- a/packages/victory-core/src/victory-portal/victory-portal.tsx +++ b/packages/victory-core/src/victory-portal/victory-portal.tsx @@ -52,3 +52,5 @@ export const VictoryPortal = (initialProps: VictoryPortalProps) => { return null; }; + +VictoryPortal.role = "portal"; diff --git a/packages/victory/src/victory.test.ts b/packages/victory/src/victory.test.ts index 3ea13f275..91f25453e 100644 --- a/packages/victory/src/victory.test.ts +++ b/packages/victory/src/victory.test.ts @@ -336,6 +336,8 @@ describe("victory", () => { "PointPathHelpers", "Portal", "PortalContext", + "PortalOutlet", + "PortalProvider", "RawZoomHelpers", "Rect", "Scale", @@ -407,6 +409,7 @@ describe("victory", () => { "makeCreateContainerFunction", "mergeRefs", "useCanvasContext", + "usePortalContext", "useVictoryBrushContainer", "useVictoryContainer", "useVictoryCursorContainer", From 56e8765f9f7a17e390db44577d1f365057d252aa Mon Sep 17 00:00:00 2001 From: Kenan Date: Fri, 17 May 2024 10:00:40 +0100 Subject: [PATCH 4/6] avoid double React.cloneElement --- packages/victory-core/src/victory-portal/portal-outlet.tsx | 4 +--- packages/victory-core/src/victory-portal/victory-portal.tsx | 1 + 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/victory-core/src/victory-portal/portal-outlet.tsx b/packages/victory-core/src/victory-portal/portal-outlet.tsx index be8b673b9..ad174d026 100644 --- a/packages/victory-core/src/victory-portal/portal-outlet.tsx +++ b/packages/victory-core/src/victory-portal/portal-outlet.tsx @@ -17,9 +17,7 @@ export const PortalOutlet = ({ }: PortalOutletProps) => { const portalContext = usePortalContext(); - const children = Array.from(portalContext.children.entries()).map( - ([key, node]) => (!node.key ? React.cloneElement(node, { key }) : node), - ); + const children = Array.from(portalContext.children.values()); return React.cloneElement(portalComponent, props, children); }; diff --git a/packages/victory-core/src/victory-portal/victory-portal.tsx b/packages/victory-core/src/victory-portal/victory-portal.tsx index 95bb66fd5..902599c1f 100644 --- a/packages/victory-core/src/victory-portal/victory-portal.tsx +++ b/packages/victory-core/src/victory-portal/victory-portal.tsx @@ -38,6 +38,7 @@ export const VictoryPortal = (initialProps: VictoryPortalProps) => { standardProps, childProps, Helpers.omit(props, ["children", "groupComponent"]), + { key: childProps.key ?? id }, ); const child = children && React.cloneElement(children, newProps); From 422b1db6543d7bd66d3f7335cbb6215e472d0d8a Mon Sep 17 00:00:00 2001 From: Kenan Date: Fri, 17 May 2024 11:01:22 +0100 Subject: [PATCH 5/6] render component in place when context is not available instead of throwing --- .../src/victory-portal/portal-context.tsx | 6 --- .../src/victory-portal/portal-outlet.tsx | 5 ++- .../src/victory-portal/victory-portal.tsx | 38 +++++++++---------- 3 files changed, 23 insertions(+), 26 deletions(-) diff --git a/packages/victory-core/src/victory-portal/portal-context.tsx b/packages/victory-core/src/victory-portal/portal-context.tsx index 10bcd95f7..1ac6a95cc 100644 --- a/packages/victory-core/src/victory-portal/portal-context.tsx +++ b/packages/victory-core/src/victory-portal/portal-context.tsx @@ -13,12 +13,6 @@ PortalContext.displayName = "PortalContext"; export const usePortalContext = () => { const context = React.useContext(PortalContext); - - if (!context) { - throw new Error( - "`usePortalContext` must be used within a ``", - ); - } return context; }; diff --git a/packages/victory-core/src/victory-portal/portal-outlet.tsx b/packages/victory-core/src/victory-portal/portal-outlet.tsx index ad174d026..0f8980757 100644 --- a/packages/victory-core/src/victory-portal/portal-outlet.tsx +++ b/packages/victory-core/src/victory-portal/portal-outlet.tsx @@ -17,7 +17,10 @@ export const PortalOutlet = ({ }: PortalOutletProps) => { const portalContext = usePortalContext(); - const children = Array.from(portalContext.children.values()); + if (!portalContext) { + return null; + } + const children = Array.from(portalContext.children.values()); return React.cloneElement(portalComponent, props, children); }; diff --git a/packages/victory-core/src/victory-portal/victory-portal.tsx b/packages/victory-core/src/victory-portal/victory-portal.tsx index 902599c1f..93bb4d9e3 100644 --- a/packages/victory-core/src/victory-portal/victory-portal.tsx +++ b/packages/victory-core/src/victory-portal/victory-portal.tsx @@ -25,33 +25,33 @@ export const VictoryPortal = (initialProps: VictoryPortalProps) => { Log.warn(msg); } + const children = Array.isArray(props.children) + ? props.children[0] + : props.children; + const { groupComponent } = props; + const childProps = (children && children.props) || {}; + const standardProps = childProps.groupComponent + ? { groupComponent, standalone: false } + : {}; + const newProps = defaults( + standardProps, + childProps, + Helpers.omit(props, ["children", "groupComponent"]), + { key: childProps.key ?? id }, + ); + const child = children && React.cloneElement(children, newProps); + React.useEffect(() => { - const children = Array.isArray(props.children) - ? props.children[0] - : props.children; - const { groupComponent } = props; - const childProps = (children && children.props) || {}; - const standardProps = childProps.groupComponent - ? { groupComponent, standalone: false } - : {}; - const newProps = defaults( - standardProps, - childProps, - Helpers.omit(props, ["children", "groupComponent"]), - { key: childProps.key ?? id }, - ); - const child = children && React.cloneElement(children, newProps); - - portalContext.addChild(id, child); + portalContext?.addChild(id, child); // eslint-disable-next-line react-hooks/exhaustive-deps }, [props.children]); React.useEffect(() => { - return () => portalContext.removeChild(id); + return () => portalContext?.removeChild(id); // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - return null; + return portalContext ? null : child; }; VictoryPortal.role = "portal"; From 7e26e9d5b0a26a080fac5206d3c4827c8f57f839 Mon Sep 17 00:00:00 2001 From: Kenan Date: Fri, 17 May 2024 11:27:07 +0100 Subject: [PATCH 6/6] move PortalProvider down tree slightly --- .../victory-container/victory-container.tsx | 36 +++++----- .../src/components/victory-container.tsx | 66 +++++++++---------- 2 files changed, 51 insertions(+), 51 deletions(-) diff --git a/packages/victory-core/src/victory-container/victory-container.tsx b/packages/victory-core/src/victory-container/victory-container.tsx index 6419c9ece..950ff86ca 100644 --- a/packages/victory-core/src/victory-container/victory-container.tsx +++ b/packages/victory-core/src/victory-container/victory-container.tsx @@ -149,22 +149,22 @@ export const VictoryContainer = (initialProps: VictoryContainerProps) => { }, []); return ( - -
+
+ { }} />
-
- + + ); }; diff --git a/packages/victory-native/src/components/victory-container.tsx b/packages/victory-native/src/components/victory-container.tsx index a349afc06..91e2a28b0 100644 --- a/packages/victory-native/src/components/victory-container.tsx +++ b/packages/victory-native/src/components/victory-container.tsx @@ -123,37 +123,37 @@ export const VictoryContainer = (initialProps: VictoryContainerNativeProps) => { const baseStyle = NativeHelpers.getStyle(style, ["width", "height"]); return ( - - + - - {/* The following Rect is a temporary solution until the following RNSVG issue is resolved https://github.com/react-native-svg/react-native-svg/issues/1488 */} - - {title ? {title} : null} - {desc ? {desc} : null} + {/* The following Rect is a temporary solution until the following RNSVG issue is resolved https://github.com/react-native-svg/react-native-svg/issues/1488 */} + + {title ? {title} : null} + {desc ? {desc} : null} + {children} { style={{ ...dimensions, overflow: "visible" }} /> - - - + + + ); };