-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat(chart-provider): add component to wrap charts #4236
Changes from all commits
3aaa9b3
6a90b9c
d7ed587
59fe16f
c395b01
d1ec0b5
ab0be7a
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@twilio-paste/codemods": minor | ||
--- | ||
|
||
[ChartProvider] added a new component that will wrap chart instances to control and share the state to child charting components |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
"@twilio-paste/chart-provider": major | ||
"@twilio-paste/core": minor | ||
--- | ||
|
||
[ChartProvider] added a new component that will wrap chart instances to control and share the state to child charting components |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import { render } from "@testing-library/react"; | ||
import { BoxProps } from "@twilio-paste/box"; | ||
import * as React from "react"; | ||
|
||
import { ChartProvider } from "../src"; | ||
|
||
const TestChartProvider: React.FC<React.PropsWithChildren<{ element?: BoxProps["element"] }>> = ({ | ||
element, | ||
children, | ||
}) => { | ||
return ( | ||
<ChartProvider highchartsOptions={{}} data-testid="chart-provider" element={element}> | ||
{children} | ||
</ChartProvider> | ||
); | ||
}; | ||
|
||
describe("ChartProvider", () => { | ||
it("should render", () => { | ||
const { getByText, getByTestId } = render(<TestChartProvider>test</TestChartProvider>); | ||
expect(getByText("test")).toBeDefined(); | ||
expect(getByTestId("chart-provider").getAttribute("data-paste-element")).toEqual("CHART_PROVIDER"); | ||
}); | ||
|
||
describe("Customization", () => { | ||
it("should apply the element prop", () => { | ||
const { getByTestId } = render(<TestChartProvider element="TEST_ELEMENT" />); | ||
expect(getByTestId("chart-provider").getAttribute("data-paste-element")).toEqual("TEST_ELEMENT"); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const { build } = require("../../../../tools/build/esbuild"); | ||
|
||
build(require("./package.json")); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
{ | ||
"name": "@twilio-paste/chart-provider", | ||
"version": "0.0.0", | ||
"category": "data display", | ||
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. Not sure exactly what category we want to use for these charting components. What do you all think? 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. unless we are adding a new category related to visualisation, data display looks good |
||
"status": "beta", | ||
"description": "Chart Provider is a data visualization component used to wrap an individual chart to store and share state to child charting elements.", | ||
"author": "Twilio Inc.", | ||
"license": "MIT", | ||
"main:dev": "src/index.tsx", | ||
"main": "dist/index.js", | ||
"module": "dist/index.es.js", | ||
"types": "dist/index.d.ts", | ||
"sideEffects": false, | ||
"publishConfig": { | ||
"access": "public" | ||
}, | ||
"files": [ | ||
"dist" | ||
], | ||
"scripts": { | ||
"build": "yarn clean && NODE_ENV=production node build.js && tsc", | ||
"build:js": "NODE_ENV=development node build.js", | ||
"build:typedocs": "tsx ../../../../tools/build/generate-type-docs", | ||
"clean": "rm -rf ./dist", | ||
"tsc": "tsc" | ||
}, | ||
"peerDependencies": { | ||
"@twilio-paste/animation-library": "^2.0.0", | ||
"@twilio-paste/box": "^10.2.0", | ||
"@twilio-paste/color-contrast-utils": "^5.0.0", | ||
"@twilio-paste/customization": "^8.1.1", | ||
"@twilio-paste/design-tokens": "^10.3.0", | ||
"@twilio-paste/style-props": "^9.1.1", | ||
"@twilio-paste/styling-library": "^3.0.0", | ||
"@twilio-paste/theme": "^11.0.1", | ||
"@twilio-paste/types": "^6.0.0", | ||
"@types/react": "^16.8.6 || ^17.0.2 || ^18.0.27", | ||
"@types/react-dom": "^16.8.6 || ^17.0.2 || ^18.0.10", | ||
"highcharts": "^9.3.3", | ||
"react": "^16.8.6 || ^17.0.2 || ^18.0.0", | ||
"react-dom": "^16.8.6 || ^17.0.2 || ^18.0.0" | ||
}, | ||
"devDependencies": { | ||
"@twilio-paste/animation-library": "^2.0.0", | ||
"@twilio-paste/box": "^10.2.0", | ||
"@twilio-paste/color-contrast-utils": "^5.0.0", | ||
"@twilio-paste/customization": "^8.1.1", | ||
"@twilio-paste/design-tokens": "^10.3.0", | ||
"@twilio-paste/style-props": "^9.1.1", | ||
"@twilio-paste/styling-library": "^3.0.0", | ||
"@twilio-paste/theme": "^11.0.1", | ||
"@twilio-paste/types": "^6.0.0", | ||
"@types/react": "^18.0.27", | ||
"@types/react-dom": "^18.0.10", | ||
"highcharts": "^9.3.3", | ||
"react": "^18.0.0", | ||
"react-dom": "^18.0.0", | ||
"tsx": "^4.0.0", | ||
"typescript": "^4.9.4" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
import * as Highcharts from "highcharts"; | ||
import * as React from "react"; | ||
|
||
export interface ChartContextProps { | ||
/** | ||
* The function that will be called by the HighchartsReact callback to set the chart object in the context. | ||
* | ||
* @param {Function} chart - the chart object returned from the HighchartsReact callback | ||
* @memberof ChartContextProps | ||
*/ | ||
setChart: (chart: Highcharts.Chart) => void; | ||
/** | ||
* Used to the set the reference to the chart element once it is populated | ||
* | ||
* @param {HTMLElement} ref - React.MutableRefObject.current of base chart component | ||
* @memberof ChartContextProps | ||
*/ | ||
setChartRef: (ref: HTMLElement) => void; | ||
/** | ||
* The options that will be passed to the ReactHighcharts component. It will be enriched with tracking events that wil be used by | ||
* other Paste components if using the ChartProvider. | ||
* | ||
* @type {Highcharts.Options} | ||
* @memberof ChartContextProps | ||
*/ | ||
options: Highcharts.Options; | ||
/** | ||
* The rendered chart returned from the HighchartsReact callback. Use this object to get the rendered properties of | ||
* series and points when calculating poitioning of custom elements. It can also be used to interact | ||
* with the chart in ways such as setting zoom levels and using chart.update to trigger changes. | ||
* | ||
* @type {Highcharts.Chart} | ||
* @memberof ChartContextProps | ||
*/ | ||
chart?: Highcharts.Chart; | ||
|
||
/** | ||
* The current reference to the base chart component. Needed for positioning custom elements relative to points. | ||
* | ||
* @type {string} | ||
* @memberof ChartContextProps | ||
*/ | ||
chartRef?: HTMLElement; | ||
/** | ||
* The current chart type. Used to trigger rerenders of other components inside ChartProvider. | ||
* | ||
* @type {string} | ||
* @memberof ChartContextProps | ||
*/ | ||
chartType?: string; | ||
Comment on lines
+45
to
+50
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. how will this string be used to trigger re-renders or other components? 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. When working on the sandbox I found that custom components that rendered based on nested elements could not pick up changes to re-render. Specifically for this I had a legend that would look at the current series and see if there was a marker symbol used to show the correct marker in the legend. When switching between chart types it would no cause a re-render due to react only shallow comparing the chart context object. Picking up the change in the chart type options and listening for that fixed the issue. It is not needed for this release so I'm happy to leave out and investigate further if there is a better way to capture this to avoid a breaking change in future. 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. I think we can leave this in since its very internal thing and probably will only cause minor change even if changed in future 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. hmm I'd push for removing it until it's necessary and we're more certain 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. I'll remove it for now, it's not being currently used. I will make the change in an upcoming commit. |
||
} | ||
|
||
/** | ||
* Setting the default values to log errors is an alternative to throwing runtime errors that still allow engineers | ||
* to debug any potential issues. | ||
*/ | ||
|
||
export const ChartContext = React.createContext<ChartContextProps>({ | ||
options: {}, | ||
setChart: () => { | ||
// eslint-disable-next-line no-console | ||
console.error("setChart not implemented. Is this component wrapped in the ChartProvider component?"); | ||
}, | ||
setChartRef: () => { | ||
// eslint-disable-next-line no-console | ||
console.error("setChartRef not implemented. Is this component wrapped in the ChartProvider component?"); | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
import { Box, safelySpreadBoxProps } from "@twilio-paste/box"; | ||
import type { BoxProps } from "@twilio-paste/box"; | ||
import type { HTMLPasteProps } from "@twilio-paste/types"; | ||
import * as Highcharts from "highcharts"; | ||
import * as React from "react"; | ||
|
||
import { ChartContext } from "./ChartContext"; | ||
|
||
interface BaseChartProviderProps extends HTMLPasteProps<"div"> { | ||
children?: React.ReactNode; | ||
/** | ||
* Overrides the default element name to apply unique styles with the Customization Provider | ||
* @default 'CHART_PROVIDER' | ||
* @type {BoxProps['element']} | ||
* @memberof ChartProviderProps | ||
*/ | ||
element?: BoxProps["element"]; | ||
} | ||
|
||
interface HighchartsOptions extends BaseChartProviderProps { | ||
/** | ||
* Overrides the default element name to apply unique styles with the Customization Provider | ||
* @default null | ||
* @type {BoxProps['element']} | ||
* @memberof ChartProviderProps | ||
*/ | ||
highchartsOptions: Highcharts.Options; | ||
pasteOptions?: never; | ||
} | ||
|
||
export type ChartProviderProps = HighchartsOptions; | ||
Comment on lines
+27
to
+31
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. A bit redundant at this point as there is only one type but sets the groundwork for union types that will only allow users to either set 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. good future thinking |
||
|
||
const ChartProvider = React.forwardRef<HTMLDivElement, ChartProviderProps>( | ||
({ element = "CHART_PROVIDER", children, highchartsOptions, ...props }, ref) => { | ||
const [chart, setChart] = React.useState<Highcharts.Chart>(); | ||
const [chartRef, setChartRef] = React.useState<HTMLElement>(); | ||
|
||
return ( | ||
<Box {...safelySpreadBoxProps(props)} ref={ref} element={element} position="relative"> | ||
<ChartContext.Provider | ||
value={{ | ||
chart, | ||
setChart, | ||
chartRef, | ||
setChartRef, | ||
options: highchartsOptions, | ||
}} | ||
> | ||
{children} | ||
</ChartContext.Provider> | ||
</Box> | ||
); | ||
}, | ||
); | ||
|
||
ChartProvider.displayName = "ChartProvider"; | ||
|
||
export { ChartProvider }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export { ChartProvider } from "./ChartProvider"; | ||
export type { ChartProviderProps } from "./ChartProvider"; | ||
export { ChartContext } from "./ChartContext"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import { Box } from "@twilio-paste/box"; | ||
import { usePasteHighchartsTheme } from "@twilio-paste/data-visualization-library"; | ||
import * as Highcharts from "highcharts"; | ||
import HighchartsReact from "highcharts-react-official"; | ||
import * as React from "react"; | ||
|
||
import { ChartContext } from "../src"; | ||
|
||
const Chart: React.FC = () => { | ||
const chartRef = React.useRef<HTMLElement | null>(null); | ||
const { options, setChart, setChartRef } = React.useContext(ChartContext); | ||
const [chartOptions, setChartOptions] = React.useState<Highcharts.Options>( | ||
usePasteHighchartsTheme({ ...options, plotOptions: { series: { animation: false } } }), | ||
); | ||
|
||
React.useLayoutEffect(() => { | ||
setChartOptions(Highcharts.merge(chartOptions, options)); | ||
}, [options]); | ||
|
||
React.useEffect(() => { | ||
if (chartRef.current) { | ||
setChartRef(chartRef.current); | ||
} | ||
}, [chartRef.current]); | ||
|
||
const callback = (chart: Highcharts.Chart): void => { | ||
if (chart?.series?.length > 0) { | ||
setChart(chart); | ||
} | ||
}; | ||
|
||
return ( | ||
<Box gridArea="base-chart" ref={chartRef} position="relative"> | ||
<HighchartsReact | ||
highcharts={Highcharts} | ||
options={chartOptions} | ||
constructorType={chartOptions.chart?.map ? "mapChart" : undefined} | ||
updateArgs={[true, true, false]} | ||
callback={callback} | ||
/> | ||
</Box> | ||
); | ||
}; | ||
|
||
export const BaseChart = React.memo(Chart); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
import type { StoryFn } from "@storybook/react"; | ||
import { Box } from "@twilio-paste/box"; | ||
import { Button } from "@twilio-paste/button"; | ||
import { Paragraph } from "@twilio-paste/paragraph"; | ||
import { Theme } from "@twilio-paste/theme"; | ||
import * as React from "react"; | ||
|
||
import { ChartContext, ChartProvider } from "../src"; | ||
import { BaseChart } from "./BaseChart"; | ||
|
||
const lineSeries: Highcharts.SeriesLineOptions[] = [ | ||
{ | ||
name: "Installation", | ||
data: [43934, 52503, 57177, 69658, 97031, 119931, 137133, 154175], | ||
type: "line", | ||
}, | ||
{ | ||
name: "Manufacturing", | ||
data: [24916, 24064, 29742, 29851, 32490, 30282, 38121, 40434], | ||
type: "line", | ||
}, | ||
{ | ||
name: "Sales & Distribution", | ||
data: [11744, 17722, 16005, 19771, 20185, 24377, 32147, 39387], | ||
type: "line", | ||
}, | ||
{ | ||
name: "Project Development", | ||
data: [null, null, 7988, 12169, 15112, 22452, 34400, 34227], | ||
type: "line", | ||
}, | ||
{ | ||
name: "Other", | ||
data: [12908, 5948, 8105, 11248, 8989, 11816, 18274, 18111], | ||
type: "line", | ||
}, | ||
]; | ||
|
||
// eslint-disable-next-line import/no-default-export | ||
export default { | ||
title: "Components/ChartProvider", | ||
}; | ||
|
||
export const Default: StoryFn = () => { | ||
return ( | ||
<ChartProvider highchartsOptions={{ chart: { type: "line" }, series: lineSeries }}> | ||
<BaseChart /> | ||
</ChartProvider> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
{ | ||
"extends": "../../../../tsconfig.json", | ||
"compilerOptions": { | ||
"outDir": "dist/", | ||
}, | ||
"include": [ | ||
"src/**/*", | ||
], | ||
"exclude": [ | ||
"node_modules" | ||
] | ||
} |
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.
question: is this assertion adding anything that's not already being tested on line 41?
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.
This is just making sure the children show up. A little unnecessary
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.
gotcha, non-blocking but would be nice to remove if it's redundant!