From cc5424eaf691157d57604bfa4550e1f999ea8bae Mon Sep 17 00:00:00 2001 From: Becca Bailey Date: Tue, 24 May 2022 16:59:13 -0700 Subject: [PATCH] Add user props to top-level container or group This moves the user props logic from the container component to the top-level returned component, whether that is a container or a group component. This allows the safe props to work in a standlone component, or a component that is a child of another victory container or VictoryChart component. --- packages/victory-area/src/victory-area.js | 10 +++--- packages/victory-bar/src/victory-bar.js | 11 +++--- .../victory-box-plot/src/victory-box-plot.js | 10 +++--- .../src/victory-candlestick.js | 11 +++--- .../victory-clip-container.js | 4 ++- .../src/victory-util/user-props.js | 13 +++++++ .../victory-errorbar/src/victory-errorbar.js | 11 +++--- .../src/victory-histogram.js | 11 +++--- packages/victory-line/src/victory-line.js | 11 +++--- packages/victory-pie/src/victory-pie.js | 11 +++--- .../victory-scatter/src/victory-scatter.js | 11 +++--- .../victory-voronoi/src/victory-voronoi.js | 11 +++--- test/jest/rendering-utils.js | 3 +- test/jest/victory-area/victory-area.test.js | 34 ++++++++++++++++--- test/jest/victory-bar/victory-bar.test.js | 22 ++++++++++-- .../victory-box-plot/victory-box-plot.test.js | 33 +++++++++++++++--- .../victory-candlestick.test.js | 33 ++++++++++++++---- test/jest/victory-line/victory-line.test.js | 32 ++++++++++++----- 18 files changed, 193 insertions(+), 89 deletions(-) diff --git a/packages/victory-area/src/victory-area.js b/packages/victory-area/src/victory-area.js index 6bacc470e..a0184514c 100644 --- a/packages/victory-area/src/victory-area.js +++ b/packages/victory-area/src/victory-area.js @@ -108,13 +108,11 @@ class VictoryArea extends React.Component { } const children = this.renderContinuousData(props); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(props) - ); - return props.standalone - ? this.renderContainer(container, children) + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } diff --git a/packages/victory-bar/src/victory-bar.js b/packages/victory-bar/src/victory-bar.js index 50ba6ee0b..600ee8499 100644 --- a/packages/victory-bar/src/victory-bar.js +++ b/packages/victory-bar/src/victory-bar.js @@ -116,13 +116,12 @@ class VictoryBar extends React.Component { } const children = this.renderData(props); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(props) - ); - return props.standalone - ? this.renderContainer(container, children) + + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } diff --git a/packages/victory-box-plot/src/victory-box-plot.js b/packages/victory-box-plot/src/victory-box-plot.js index 924db2143..fe24bc466 100644 --- a/packages/victory-box-plot/src/victory-box-plot.js +++ b/packages/victory-box-plot/src/victory-box-plot.js @@ -309,13 +309,11 @@ class VictoryBoxPlot extends React.Component { } const children = this.renderBoxPlot(props); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(props) - ); - return props.standalone - ? this.renderContainer(container, children) + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } diff --git a/packages/victory-candlestick/src/victory-candlestick.js b/packages/victory-candlestick/src/victory-candlestick.js index 2fce52bf9..769f0d9b9 100644 --- a/packages/victory-candlestick/src/victory-candlestick.js +++ b/packages/victory-candlestick/src/victory-candlestick.js @@ -296,13 +296,12 @@ class VictoryCandlestick extends React.Component { } const children = this.renderCandleData(props, this.shouldRenderDatum); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(props) - ); - return props.standalone - ? this.renderContainer(container, children) + + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } diff --git a/packages/victory-core/src/victory-clip-container/victory-clip-container.js b/packages/victory-core/src/victory-clip-container/victory-clip-container.js index 51d8d63ce..96f4612ef 100644 --- a/packages/victory-core/src/victory-clip-container/victory-clip-container.js +++ b/packages/victory-core/src/victory-clip-container/victory-clip-container.js @@ -2,6 +2,7 @@ import React from "react"; import PropTypes from "prop-types"; import * as CustomPropTypes from "../victory-util/prop-types"; import * as Helpers from "../victory-util/helpers"; +import * as UserProps from "../victory-util/user-props"; import { assign, defaults, isObject, uniqueId } from "lodash"; import ClipPath from "../victory-primitives/clip-path"; import Circle from "../victory-primitives/circle"; @@ -84,6 +85,7 @@ export default class VictoryClipContainer extends React.Component { } renderClippedGroup(props, clipId) { + const userProps = UserProps.getSafeUserProps(props); const { style, events, @@ -106,7 +108,7 @@ export default class VictoryClipContainer extends React.Component { ); return React.cloneElement( groupComponent, - { ...groupProps, "aria-label": props["aria-label"], tabIndex }, + { ...groupProps, tabIndex, ...userProps }, [clipComponent, ...React.Children.toArray(children)] ); } diff --git a/packages/victory-core/src/victory-util/user-props.js b/packages/victory-core/src/victory-util/user-props.js index d502e73c5..ec7076d80 100644 --- a/packages/victory-core/src/victory-util/user-props.js +++ b/packages/victory-core/src/victory-util/user-props.js @@ -1,3 +1,5 @@ +import * as React from "react"; + /* USER_PROPS_SAFELIST is to contain any string deemed safe for user props. The startsWidth array will contain the start of any accepted user-prop that @@ -79,3 +81,14 @@ export const getSafeUserProps = (props) => { }) ); }; + +/** + * Wraps a component and adds safe user props + * + * @param {ReactNode} component: parent component + * @param {Object} props: props to be filtered + * @returns {ReactNode} modified component + */ +export const withSafeUserProps = (component, props) => { + return React.cloneElement(component, getSafeUserProps(props)); +}; diff --git a/packages/victory-errorbar/src/victory-errorbar.js b/packages/victory-errorbar/src/victory-errorbar.js index 1666bcd96..25b4a2e94 100644 --- a/packages/victory-errorbar/src/victory-errorbar.js +++ b/packages/victory-errorbar/src/victory-errorbar.js @@ -106,13 +106,12 @@ class VictoryErrorBar extends React.Component { } const children = this.renderData(props); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(props) - ); - return props.standalone - ? this.renderContainer(container, children) + + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } diff --git a/packages/victory-histogram/src/victory-histogram.js b/packages/victory-histogram/src/victory-histogram.js index 4114a43eb..a3dd199ba 100644 --- a/packages/victory-histogram/src/victory-histogram.js +++ b/packages/victory-histogram/src/victory-histogram.js @@ -122,14 +122,11 @@ export class VictoryHistogram extends React.Component { const children = this.renderData(props); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(this.props) - ); - - return props.standalone - ? this.renderContainer(container, children) + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } diff --git a/packages/victory-line/src/victory-line.js b/packages/victory-line/src/victory-line.js index a4dff0e60..c7c27135a 100644 --- a/packages/victory-line/src/victory-line.js +++ b/packages/victory-line/src/victory-line.js @@ -111,13 +111,12 @@ class VictoryLine extends React.Component { } const children = this.renderContinuousData(props); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(props) - ); - return props.standalone - ? this.renderContainer(container, children) + + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } export default addEvents(VictoryLine, options); diff --git a/packages/victory-pie/src/victory-pie.js b/packages/victory-pie/src/victory-pie.js index a0fa954c2..c95faacd6 100644 --- a/packages/victory-pie/src/victory-pie.js +++ b/packages/victory-pie/src/victory-pie.js @@ -256,13 +256,12 @@ class VictoryPie extends React.Component { } const children = this.renderData(props); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(props) - ); - return props.standalone - ? this.renderContainer(container, children) + + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } diff --git a/packages/victory-scatter/src/victory-scatter.js b/packages/victory-scatter/src/victory-scatter.js index 76f59c468..cd9e9b04f 100644 --- a/packages/victory-scatter/src/victory-scatter.js +++ b/packages/victory-scatter/src/victory-scatter.js @@ -99,13 +99,12 @@ class VictoryScatter extends React.Component { } const children = this.renderData(props); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(props) - ); - return props.standalone - ? this.renderContainer(container, children) + + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } diff --git a/packages/victory-voronoi/src/victory-voronoi.js b/packages/victory-voronoi/src/victory-voronoi.js index 17a96464e..b6f2b8abd 100644 --- a/packages/victory-voronoi/src/victory-voronoi.js +++ b/packages/victory-voronoi/src/victory-voronoi.js @@ -78,13 +78,12 @@ class VictoryVoronoi extends React.Component { } const children = this.renderData(props); - const container = React.cloneElement( - props.containerComponent, - UserProps.getSafeUserProps(props) - ); - return props.standalone - ? this.renderContainer(container, children) + + const component = props.standalone + ? this.renderContainer(props.containerComponent, children) : children; + + return UserProps.withSafeUserProps(component, props); } } diff --git a/test/jest/rendering-utils.js b/test/jest/rendering-utils.js index 8890b255a..d42446086 100644 --- a/test/jest/rendering-utils.js +++ b/test/jest/rendering-utils.js @@ -1,6 +1,5 @@ import { render } from "@testing-library/react"; -import * as React from "react"; export const renderInSvg = (component) => { - return render({component}); + return render(component, { wrapper: "svg" }); }; diff --git a/test/jest/victory-area/victory-area.test.js b/test/jest/victory-area/victory-area.test.js index 84e62c8fd..625fbedbf 100644 --- a/test/jest/victory-area/victory-area.test.js +++ b/test/jest/victory-area/victory-area.test.js @@ -1,17 +1,43 @@ +import "@testing-library/jest-dom"; import { fireEvent, render, screen } from "@testing-library/react"; -import { curveCatmullRom } from "victory-vendor/d3-shape"; import { range } from "lodash"; import React from "react"; import { Area, VictoryArea } from "victory-area"; +import { VictoryChart } from "victory-chart"; +import { curveCatmullRom } from "victory-vendor/d3-shape"; import { calculateD3Path } from "../../svg-test-helper"; describe("components/victory-area", () => { describe("default component rendering", () => { - it("accepts user props", () => { - render(); + it("attaches safe user props to the container component", () => { + render( + + ); + + const container = screen.getByTestId("victory-area"); + expect(screen.getByLabelText("Chart")).toBeDefined(); + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.nodeName).toEqual("svg"); + }); + + it("attaches safe user props to the group component if the component is rendered inside a VictoryChart", () => { + render( + , + { wrapper: VictoryChart } + ); - expect(screen.queryByTestId("victory-area")).toBeDefined(); + const container = screen.getByTestId("victory-area"); expect(screen.getByLabelText("Chart")).toBeDefined(); + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.nodeName).toEqual("g"); }); it("renders an svg with the correct viewbox", () => { diff --git a/test/jest/victory-bar/victory-bar.test.js b/test/jest/victory-bar/victory-bar.test.js index 39da8c4dd..50bad6606 100644 --- a/test/jest/victory-bar/victory-bar.test.js +++ b/test/jest/victory-bar/victory-bar.test.js @@ -3,6 +3,7 @@ import React from "react"; import { render, fireEvent, screen } from "@testing-library/react"; import { range } from "lodash"; +import { VictoryChart } from "victory-chart"; import { VictoryBar, Bar } from "victory-bar"; import { isBar, getBarHeight } from "../../svg-test-helper"; import "@testing-library/jest-dom"; @@ -18,11 +19,26 @@ describe("components/victory-bar", () => { /> ); - expect(screen.getByTestId("victory-bar")).toBeDefined(); + const container = screen.getByTestId("victory-bar"); expect(screen.getByLabelText("Chart")).toBeDefined(); - expect(screen.getByTestId("victory-bar")).not.toHaveAttribute( - "unsafe-prop" + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.tagName).toEqual("svg"); + }); + + it("attaches safe user props to the group component if the component is rendered inside a VictoryChart", () => { + render( + , + { wrapper: VictoryChart } ); + + const container = screen.getByTestId("victory-bar"); + expect(screen.getByLabelText("Chart")).toBeDefined(); + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.tagName).toEqual("g"); }); it("renders an svg with the correct width and height", () => { diff --git a/test/jest/victory-box-plot/victory-box-plot.test.js b/test/jest/victory-box-plot/victory-box-plot.test.js index 2505c30f1..5b00f0e4d 100644 --- a/test/jest/victory-box-plot/victory-box-plot.test.js +++ b/test/jest/victory-box-plot/victory-box-plot.test.js @@ -1,8 +1,10 @@ /*eslint-disable react/prop-types */ -import React from "react"; +import "@testing-library/jest-dom"; import { render, screen } from "@testing-library/react"; +import React from "react"; import { VictoryBoxPlot } from "victory-box-plot"; -import { LineSegment, Whisker, Border } from "victory-core"; +import { VictoryChart } from "victory-chart"; +import { Border, LineSegment, Whisker } from "victory-core"; const TEST_GROUP_ID = "test-group-id"; const dataset = [ @@ -26,12 +28,35 @@ const renderWithTestGroup = (data = dataset) => { describe("components/victory-box-plot", () => { describe("default component rendering", () => { - it("accepts user props", () => { + it("attaches safe user props to the container component", () => { render( - + + ); + + const container = screen.getByTestId("victory-boxplot"); + expect(screen.getByLabelText("Chart")).toBeDefined(); + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.nodeName).toEqual("svg"); + }); + + it("attaches safe user props to the group component if the component is rendered inside a VictoryChart", () => { + render( + , + { wrapper: VictoryChart } ); + const container = screen.getByTestId("victory-boxplot"); expect(screen.getByLabelText("Chart")).toBeDefined(); + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.nodeName).toEqual("g"); }); it("renders an svg with the correct width and height", () => { diff --git a/test/jest/victory-candlestick/victory-candlestick.test.js b/test/jest/victory-candlestick/victory-candlestick.test.js index 858439e91..7c1f87429 100644 --- a/test/jest/victory-candlestick/victory-candlestick.test.js +++ b/test/jest/victory-candlestick/victory-candlestick.test.js @@ -1,8 +1,10 @@ /*eslint-disable max-nested-callbacks */ -import React from "react"; -import { render, screen, fireEvent } from "@testing-library/react"; +import "@testing-library/jest-dom"; +import { fireEvent, render, screen } from "@testing-library/react"; import { range } from "lodash"; -import { VictoryCandlestick, Candle } from "victory-candlestick"; +import React from "react"; +import { Candle, VictoryCandlestick } from "victory-candlestick"; +import { VictoryChart } from "victory-chart"; const MyCandle = () =>
; @@ -13,16 +15,35 @@ const dataSet = [ describe("components/victory-candlestick", () => { describe("default component rendering", () => { - it("accepts user props", () => { + it("attaches safe user props to the container component", () => { render( ); - const svgNode = screen.getByTestId("victory-candlestick"); - expect(svgNode.getAttribute("aria-label")).toEqual("Chart"); + const container = screen.getByTestId("victory-candlestick"); + expect(screen.getByLabelText("Chart")).toBeDefined(); + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.nodeName).toEqual("svg"); + }); + + it("attaches safe user props to the group component if the component is rendered inside a VictoryChart", () => { + render( + , + { wrapper: VictoryChart } + ); + + const container = screen.getByTestId("victory-candlestick"); + expect(screen.getByLabelText("Chart")).toBeDefined(); + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.nodeName).toEqual("g"); }); it("renders an svg with the correct width and height", () => { diff --git a/test/jest/victory-line/victory-line.test.js b/test/jest/victory-line/victory-line.test.js index c647e86bd..8820ff72b 100644 --- a/test/jest/victory-line/victory-line.test.js +++ b/test/jest/victory-line/victory-line.test.js @@ -1,10 +1,11 @@ -import React from "react"; +import "@testing-library/jest-dom"; import { fireEvent, render, screen } from "@testing-library/react"; -import { VictoryLine, Curve } from "victory-line"; -import { calculateD3Path } from "../../svg-test-helper"; +import { random, range } from "lodash"; +import React from "react"; +import { VictoryChart } from "victory-chart"; +import { Curve, VictoryLine } from "victory-line"; import { curveCatmullRom } from "victory-vendor/d3-shape"; -import { range, random } from "lodash"; -import "@testing-library/jest-dom"; +import { calculateD3Path } from "../../svg-test-helper"; describe("components/victory-line", () => { describe("default component rendering", () => { @@ -17,11 +18,26 @@ describe("components/victory-line", () => { /> ); - expect(screen.getByTestId("victory-line")).toBeDefined(); + const container = screen.getByTestId("victory-line"); expect(screen.getByLabelText("Chart")).toBeDefined(); - expect(screen.getByTestId("victory-line")).not.toHaveAttribute( - "unsafe-prop" + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.nodeName).toEqual("svg"); + }); + + it("attaches safe user props to the group component if the component is rendered inside a VictoryChart", () => { + render( + , + { wrapper: VictoryChart } ); + + const container = screen.getByTestId("victory-line"); + expect(screen.getByLabelText("Chart")).toBeDefined(); + expect(container).not.toHaveAttribute("unsafe-prop"); + expect(container.tagName).toEqual("g"); }); it("renders an svg with the correct viewBox", () => {