-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[charts][POC] Charts #20361
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
base: master
Are you sure you want to change the base?
[charts][POC] Charts #20361
Changes from all commits
447e623
c949717
07ab055
8a9c79b
0eb3908
2c447de
da31c1d
9a1e233
0d7a18c
66637d0
06365cc
f339c25
17d7523
c20dbc3
92870d3
dd7b3e9
80c8772
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 @@ | ||
| import * as React from 'react'; | ||
|
|
||
| export default function PocBaseAndMaterial() { | ||
| return <div>PocBaseAndMaterial</div>; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import * as React from 'react'; | ||
| import { PieChart as PieChartBase } from '@mui/x-charts-base'; | ||
| import { PieChart as PieChartMaterial } from '@mui/x-charts-material'; | ||
| import type { PieChartProps } from '@mui/x-charts/PieChart'; | ||
|
|
||
| const data: PieChartProps = { | ||
| height: 300, | ||
| enableKeyboardNavigation: true, | ||
| series: [ | ||
| { | ||
| arcLabel: 'value', | ||
| arcLabelMinAngle: 10, | ||
| innerRadius: '70%', | ||
| data: [ | ||
| { value: 15, label: 'A' }, | ||
| { value: 20, label: 'B' }, | ||
| ], | ||
| }, | ||
| { | ||
| outerRadius: '70%', | ||
| innerRadius: '40%', | ||
| cx: '100%', | ||
| startAngle: 180, | ||
| arcLabel: 'value', | ||
| data: [ | ||
| { value: 15, label: 'D', color: 'rgb(135, 120, 255)' }, | ||
| { value: 25, label: 'E', color: 'rgb(160, 143, 255)' }, | ||
| { value: 35, label: 'F', color: 'rgb(185, 166, 255)' }, | ||
| ], | ||
| }, | ||
| { | ||
| outerRadius: '70%', | ||
| innerRadius: '40%', | ||
| cx: '0%', | ||
| endAngle: 180, | ||
| arcLabel: 'value', | ||
| data: [ | ||
| { value: 15, label: 'D', color: 'rgb(255, 194, 163)' }, | ||
| { value: 25, label: 'E', color: 'rgb(255, 186, 138)' }, | ||
| { value: 35, label: 'F', color: 'rgb(255, 177, 112)' }, | ||
| ], | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| export default function PocBaseAndMaterial() { | ||
| return ( | ||
| <div> | ||
| <PieChartBase.Root {...data}> | ||
| <PieChartBase.Surface> | ||
|
Comment on lines
+49
to
+50
Member
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 about naming every thing with Mostly because the container and the surface can be shared between charts. Having dedicated namings for common aspect feels more natural <Common.Root>
<Common.Surface>
<Cartesian.Grid />
<LineChart.LinePlot />
<BarChart.BarPlot />
<Cartesian.XAxis />
<Cartesian.YAxis />
</Common.Surface>
</Common.Root>Maybe it's just me who gets too old, but Root does nto seems trivial to know if it's just a provider or a dom elements that does something
Member
Author
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 for cartesian specific parts it could makes sense to bundle them into a |
||
| <PieChartBase.Plot /> | ||
| <PieChartBase.LabelPlot /> | ||
| </PieChartBase.Surface> | ||
| </PieChartBase.Root> | ||
| <PieChartMaterial {...data} /> | ||
| </div> | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -269,3 +269,12 @@ | |
| The bell curve is calculated based on the mean and standard deviation of the data. | ||
|
|
||
| {{"demo": "BellCurveOverlay.js" }} | ||
|
|
||
| ### POC: Base and Material UI packages | ||
|
Check failure on line 273 in docs/data/charts/composition/composition.md
|
||
|
|
||
| This example demonstrates the proposed architecture where charts functionality is split into two packages: | ||
|
|
||
| - **@mui/x-charts-base**: Framework-agnostic core with inline styles and a custom theme system | ||
|
Member
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. We shouldn't have any theme system in the headless part, it should come from the styled layer
Member
Author
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. Yeah kinda true, but we will still need a way to say what are the colors the chart should use through css classes/attributes/cssvars, this is a "theme system" IMO, even if we don't provide default values
Member
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 thought the same but now I can see an option where no color are provided. The chart could be fully dark, the same way as base-ui elements are ugly when you remove the CSS from the demo. From now when I look at the usage of
We could introduce CSS vars to define for example the color of axes to simplify users life. But that open questions on our side like "Does axis tick should have the same color as the axis main line? Same for ticks labels, or axis label". At the end, it might be easier for the user to define a
Member
Author
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 don't get what you mean here.
Member
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. It might be easier for users to do the styling from scratch once than to customize a default theming solution. My .axisRoot {
stroke: "var(--my-theme-dark-black)",
text: {
stroke: "non",
fill: "var(--my-theme-light-black)",
}
.axisLabel: {
fill: "var(--my-theme-colorfull-text)",
}
.tick: {
stroke: "var(--my-theme-light-black)",
}
}It's a bit more work than having something that work out of the box, but we leave full flexibility to the user, and that's probably what they are looking for if they use headless solution |
||
| - **@mui/x-charts-material**: Material UI integration layer using material components and theming | ||
|
Check failure on line 278 in docs/data/charts/composition/composition.md
|
||
|
Member
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. Alongside the Material UI version, it would be nice to build a Plain CSS demo of the same component to see if we are not by any chance leaking any Material UI concepts in the headless pat. |
||
|
|
||
| {{"demo": "PocBaseAndMaterial.js" }} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| { | ||
| "name": "@mui/x-charts-base", | ||
| "version": "0.0.1", | ||
| "description": "Framework-agnostic base charting library for MUI X Charts", | ||
| "author": "MUI Team", | ||
| "main": "src/index.ts", | ||
| "license": "MIT", | ||
| "private": true, | ||
| "bugs": { | ||
| "url": "https://github.com/mui/mui-x/issues" | ||
| }, | ||
| "homepage": "https://mui.com/x/react-charts/", | ||
| "funding": { | ||
| "type": "opencollective", | ||
| "url": "https://opencollective.com/mui-org" | ||
| }, | ||
| "scripts": { | ||
| "build": "pnpm build:modern && pnpm build:node && pnpm build:stable && pnpm build:types && pnpm build:copy-files", | ||
| "build:modern": "node ../../scripts/build.mjs modern", | ||
| "build:node": "node ../../scripts/build.mjs node", | ||
| "build:stable": "node ../../scripts/build.mjs stable", | ||
| "build:copy-files": "node ../../scripts/copyFiles.mjs", | ||
| "build:types": "node ../../scripts/buildTypes.mjs", | ||
| "typescript": "tsc -p tsconfig.json" | ||
| }, | ||
| "dependencies": { | ||
| "@babel/runtime": "catalog:", | ||
| "@mui/utils": "catalog:", | ||
| "@mui/material": "catalog:", | ||
|
Comment on lines
+28
to
+29
Member
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 know we are experimenting, but these dependencies should go out. We can go one by one and see what we use from these packages and either make a copy or move that logic in the styled layer.
Member
Author
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. Yeah, these are just examples, so I didn't want to fully bring a lot of helpers we use. I don't think it was valuable for this example to bring all our plugin system for example, since they are just JS anyways. So I just imported the necessary functions. This allows us to focus the effort on thinking how the actual React/Style API would be used instead. |
||
| "@mui/x-charts-vendor": "workspace:^", | ||
| "@mui/x-internals": "workspace:^", | ||
| "@mui/x-charts": "workspace:^", | ||
|
Comment on lines
+30
to
+32
Member
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. Are these "styled"-less packages?
Member
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.
|
||
| "clsx": "catalog:" | ||
| }, | ||
| "peerDependencies": { | ||
| "react": "^17.0.0 || ^18.0.0 || ^19.0.0", | ||
| "react-dom": "^17.0.0 || ^18.0.0 || ^19.0.0" | ||
| }, | ||
| "devDependencies": {}, | ||
| "sideEffects": false, | ||
| "publishConfig": { | ||
| "access": "public", | ||
| "directory": "build" | ||
| }, | ||
| "engines": { | ||
| "node": ">=14.0.0" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| 'use client'; | ||
| import * as React from 'react'; | ||
| import useForkRef from '@mui/utils/useForkRef'; | ||
| import { useSvgRef } from '@mui/x-charts'; | ||
| import { ChartsAxesGradients } from '@mui/x-charts/internals/components/ChartsAxesGradients'; | ||
| import { | ||
| selectorChartSvgWidth, | ||
| selectorChartSvgHeight, | ||
| selectorChartPropsWidth, | ||
| selectorChartPropsHeight, | ||
| } from '@mui/x-charts/internals/plugins/corePlugins/useChartDimensions'; | ||
| import { | ||
| selectorChartsIsKeyboardNavigationEnabled, | ||
| selectorChartsHasFocusedItem, | ||
| } from '@mui/x-charts/internals/plugins/featurePlugins/useChartKeyboardNavigation'; | ||
| import { useSelector } from '@mui/x-charts/internals/store/useSelector'; | ||
| import clsx from 'clsx'; | ||
| import { useStore } from '@mui/x-charts/internals/store/useStore'; | ||
|
|
||
| export interface ChartsSurfaceProps | ||
| extends Omit< | ||
| React.SVGProps<SVGSVGElement>, | ||
| 'id' | 'children' | 'className' | 'height' | 'width' | 'cx' | 'cy' | 'viewBox' | 'color' | 'ref' | ||
| > { | ||
| className?: string; | ||
| title?: string; | ||
| desc?: string; | ||
| children?: React.ReactNode; | ||
| } | ||
|
|
||
| const ChartsSurface = React.forwardRef<SVGSVGElement, ChartsSurfaceProps>(function ChartsSurface( | ||
| inProps: ChartsSurfaceProps, | ||
| ref: React.Ref<SVGSVGElement>, | ||
| ) { | ||
| const store = useStore(); | ||
|
|
||
| const svgWidth = useSelector(store, selectorChartSvgWidth); | ||
| const svgHeight = useSelector(store, selectorChartSvgHeight); | ||
|
|
||
| const propsWidth = useSelector(store, selectorChartPropsWidth); | ||
| const propsHeight = useSelector(store, selectorChartPropsHeight); | ||
| const isKeyboardNavigationEnabled = useSelector(store, selectorChartsIsKeyboardNavigationEnabled); | ||
| const hasFocusedItem = useSelector(store, selectorChartsHasFocusedItem); | ||
| const svgRef = useSvgRef(); | ||
| const handleRef = useForkRef(svgRef, ref); | ||
|
|
||
| const { children, className, title, desc, ...other } = inProps; | ||
|
|
||
| const hasIntrinsicSize = svgHeight > 0 && svgWidth > 0; | ||
|
|
||
| const styles = { | ||
| ...(propsWidth ? { '--chart-surface-width': `${propsWidth}px` } : {}), | ||
| ...(propsHeight ? { '--chart-surface-height': `${propsHeight}px` } : {}), | ||
| } as React.CSSProperties; | ||
|
|
||
| return ( | ||
| <svg | ||
| viewBox={`${0} ${0} ${svgWidth} ${svgHeight}`} | ||
| className={clsx('ChartsSurface-root', className)} | ||
| tabIndex={isKeyboardNavigationEnabled ? 0 : undefined} | ||
| data-has-focused-item={hasFocusedItem || undefined} | ||
| style={styles} | ||
| {...other} | ||
| ref={handleRef} | ||
| > | ||
| {title && <title>{title}</title>} | ||
| {desc && <desc>{desc}</desc>} | ||
| <ChartsAxesGradients /> | ||
| {hasIntrinsicSize && children} | ||
| </svg> | ||
| ); | ||
| }); | ||
|
|
||
| export { ChartsSurface }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export * from './ChartsSurface'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { styled } from '@mui/material'; | ||
|
|
||
| export const FakeCss = styled('div')(({ theme }) => ({ | ||
|
Member
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. Is this the only CSS we need? If yes, it's great! :)
Member
Author
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. So far, yeah |
||
| '& .ChartsSurface-root': { | ||
| height: 'var(--chart-surface-height, 100%)', | ||
| width: 'var(--chart-surface-width, 100%)', | ||
|
Comment on lines
+5
to
+6
Member
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. We should just provide the CSS variabels values in the style tag this is how those should be consumed.
Member
Author
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. What do you mean? |
||
| display: 'flex', | ||
| position: 'relative', | ||
| flexDirection: 'column', | ||
| alignItems: 'center', | ||
| justifyContent: 'center', | ||
| overflow: 'hidden', | ||
|
Comment on lines
+7
to
+12
Member
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. This should be in the styled layer. |
||
| /* This prevents default touch actions when using the svg on mobile devices. | ||
| For example, prevent page scroll & zoom. */ | ||
| touchAction: 'pan-y', | ||
| userSelect: 'none', | ||
| gridArea: 'chart', | ||
|
Comment on lines
+15
to
+17
Member
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. If these are functional styles that we should always have they should be directly set on the element via the style tag. If not, it's enough to have them in a demo. |
||
| }, | ||
|
|
||
| '& .ChartsSurface-root:focus': { | ||
|
Member
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. Everything from here to the bottom should go in the styled layer/demo. Also make sure that all these elements are accessible on the styled layer, e.g. either as a component or something the user has to render. |
||
| outline: 'none', // By default don't show focus on the SVG container | ||
| }, | ||
|
|
||
| '& .ChartsSurface-root:focus-visible': { | ||
| /* Show focus outline on the SVG container only when using keyboard navigation */ | ||
| outline: 'var(--mui-palette-text-primary) solid 2px', | ||
| }, | ||
|
|
||
| "& .ChartsSurface-root:focus-visible[data-has-focused-item='true']": { | ||
| /* But not if the chart has a focused children item */ | ||
| outline: 'none', | ||
| }, | ||
|
|
||
| "& .ChartsSurface-root [data-focused='true']": { | ||
| outline: 'var(--mui-palette-text-primary) solid 2px', | ||
| }, | ||
|
|
||
| // @media doesn't work | ||
| '@media (prefers-color-scheme: dark)': {}, | ||
|
|
||
| '--Primary-color': 'orange', | ||
|
|
||
| '--PieChart-arc-stroke': 'white', | ||
| ...theme.applyStyles('dark', { | ||
| '--PieChart-arc-stroke': 'black', | ||
| }), | ||
|
|
||
| '--FocusIndicator-color': 'var(--Primary-color)', | ||
|
Member
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. No theme leaks, e.g. non Material UI libraries may not even have this CSS var, or it could look differently.
Member
Author
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. Yeah but this is used internally, not externally, Eg: A user intending to set all focus indicator to the same color could simply use --FocusIndicator-color: pink |
||
| })); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| 'use client'; | ||
| import { type PieItemId } from '@mui/x-charts'; | ||
| import { useInteractionItemProps } from '@mui/x-charts/hooks/useInteractionItemProps'; | ||
| import clsx from 'clsx'; | ||
| import * as React from 'react'; | ||
| import { arc as d3Arc } from '@mui/x-charts-vendor/d3-shape'; | ||
|
|
||
| export type PieArcProps = Omit<React.SVGProps<SVGPathElement>, 'ref' | 'id'> & { | ||
| cornerRadius: number; | ||
| endAngle: number; | ||
| innerRadius: number; | ||
| onClick?: (event: React.MouseEvent<SVGPathElement, MouseEvent>) => void; | ||
| outerRadius: number; | ||
| paddingAngle: number; | ||
| startAngle: number; | ||
| /** | ||
| * If `true`, the default event handlers are disabled. | ||
| * Those are used, for example, to display a tooltip or highlight the arc on hover. | ||
| */ | ||
| skipInteraction?: boolean; | ||
| id: PieItemId; | ||
| dataIndex: number; | ||
| color: string; | ||
| isFaded: boolean; | ||
| isHighlighted: boolean; | ||
| isFocused: boolean; | ||
| stroke?: string; | ||
| }; | ||
|
|
||
| const PieArc = React.forwardRef<SVGPathElement, PieArcProps>(function PieArc(props, ref) { | ||
| const { | ||
| className, | ||
| color, | ||
| dataIndex, | ||
| id, | ||
| isFaded, | ||
| isHighlighted, | ||
| isFocused, | ||
| onClick, | ||
| cornerRadius, | ||
| startAngle, | ||
| endAngle, | ||
| innerRadius, | ||
| outerRadius, | ||
| paddingAngle, | ||
| skipInteraction, | ||
| stroke, | ||
| ...other | ||
| } = props; | ||
|
|
||
| const interactionProps = useInteractionItemProps( | ||
| { type: 'pie', seriesId: id, dataIndex }, | ||
| skipInteraction, | ||
| ); | ||
| const p = { | ||
| cornerRadius, | ||
| startAngle, | ||
| endAngle, | ||
| innerRadius, | ||
| outerRadius, | ||
| paddingAngle, | ||
| }; | ||
|
|
||
| const d = d3Arc().cornerRadius(p.cornerRadius)({ | ||
| padAngle: p.paddingAngle, | ||
| innerRadius: p.innerRadius, | ||
| outerRadius: p.outerRadius, | ||
| startAngle: p.startAngle, | ||
| endAngle: p.endAngle, | ||
| })!; | ||
| const visibility = p.startAngle === p.endAngle ? ('hidden' as const) : ('visible' as const); | ||
|
|
||
| return ( | ||
| <path | ||
| ref={ref} | ||
| onClick={onClick} | ||
| cursor={onClick ? 'pointer' : 'unset'} | ||
| className={clsx('PieArc-root', className)} | ||
| fill={color} | ||
| opacity={isFaded ? 0.3 : 1} | ||
| filter={isHighlighted ? 'brightness(120%)' : 'none'} | ||
| stroke={stroke} | ||
| strokeWidth={1} | ||
| strokeLinejoin="round" | ||
| data-highlighted={isHighlighted || undefined} | ||
| data-faded={isFaded || undefined} | ||
| d={d} | ||
| visibility={visibility} | ||
| {...other} | ||
| {...interactionProps} | ||
| /> | ||
| ); | ||
| }); | ||
|
|
||
| export { PieArc }; |
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.
I'm not sure about calling them
PieChartBase.RootandPieChartBase.SurfaceMostly because we would end up with a mix of line and bar when using composition
Maybe something more like that
it's maybe just because I'm not used to it, but passing everything to something called Root feel weird
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.
I believe this is the pattern joy/base use, so we might as well keep with it.
https://joy-ui.com/dialog/
https://base-ui.com/react/components/avatar
My idea is to provide this for every chart in case you want to use the correct types for a specific chart.
IMO it works better because we have composable and non-composable charts, so we can create a clear pattern where we always use the
ChartName.Root/Surface, and then if we want composable we can useComposition.Root/Surface