-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(react-charts): vega lite schema support #35546
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?
feat(react-charts): vega lite schema support #35546
Conversation
📊 Bundle size report✅ No changes found |
|
Pull request demo site: URL |
| @@ -0,0 +1,247 @@ | |||
| # Vega-Lite Schema Integration Summary | |||
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.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/CalendarCompat 4 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/CalendarCompat.multiDayView - High Contrast.default.chromium.png | 675 | Changed |
| vr-tests-react-components/CalendarCompat.multiDayView - Dark Mode.default.chromium.png | 551 | Changed |
| vr-tests-react-components/CalendarCompat.multiDayView - RTL.default.chromium.png | 390 | Changed |
| vr-tests-react-components/CalendarCompat.multiDayView.default.chromium_1.png | 478 | Changed |
vr-tests-react-components/Charts-DonutChart 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Charts-DonutChart.Dynamic - RTL.default.chromium.png | 5570 | Changed |
vr-tests-react-components/Positioning 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| vr-tests-react-components/Positioning.Positioning end.chromium.png | 722 | Changed |
| vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 29 | Changed |
There were 1 duplicate changes discarded. Check the build logs for more information.
- Use resetIdsForTests() from @fluentui/react-utilities (same as Plotly tests) - Update snapshots with deterministic IDs - Update schema count test from >100 to 25 (after schema reduction) - All VegaDeclarativeChart tests now pass Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Use resetIdsForTests() from @fluentui/react-utilities (same as Plotly tests) - Update snapshots with deterministic IDs - Update schema count test from >100 to 25 (after schema reduction) - All VegaDeclarativeChart tests now pass Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ayMsft/fluentui into usr/atisjai/vegaLiteFluent
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.
Are these change summary reports needed?
| /** | ||
| * Helper to compare two arrays for equality | ||
| */ | ||
| function arraysEqual(a: string[], b: string[]): boolean { |
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 function can be imported from the utilities:
fluentui/packages/charts/react-charts/library/src/utilities/utilities.ts
Lines 1978 to 1991 in 72fa991
| export function areArraysEqual(arr1?: string[], arr2?: string[]): boolean { | |
| if (arr1 === arr2 || (!arr1 && !arr2)) { | |
| return true; | |
| } | |
| if (!arr1 || !arr2 || arr1.length !== arr2.length) { | |
| return false; | |
| } | |
| for (let i = 0; i < arr1.length; i++) { | |
| if (arr1[i] !== arr2[i]) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } |
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.
Would it be better to keep these adapters inside the VegaDeclarativeChart folder?
| // Inline schemas (25 total covering various chart types) | ||
| // These are the default schemas shown in "show few" mode | ||
| const ALL_SCHEMAS: Record<string, any> = { | ||
| adCtrScatter: { |
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.
These schemas can be stringified to reduce LOC.
| // These are the default schemas shown in "show few" mode | ||
| const ALL_SCHEMAS: Record<string, any> = { | ||
| adCtrScatter: { | ||
| $schema: 'https://vega.github.io/schema/vega-lite/v5.json', |
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.
Would it be helpful to include the recommended Vega schema version in the JSDoc comments and the example docs?
| ALL_OPTIONS.sort((a, b) => { | ||
| if (a.category !== b.category) { | ||
| // Priority order for categories | ||
| const categoryOrder = [ |
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.
Can be extracted for reuse
| name.includes('expense') || | ||
| name.includes('roi') || | ||
| name.includes('financial') || | ||
| name.includes('dividend') |
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.
Can be simplified to:
['stock', 'portfolio', 'profit', 'revenue', 'cashflow', 'budget', 'expense', 'roi', 'financial', 'dividend'].some(keyword => name.includes(keyword))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.
Would it be better to combine these 3 test files into 1 since they all contain component tests?
| /** | ||
| * Hook to determine if dark theme is active | ||
| */ | ||
| function useIsDarkTheme(): boolean { |
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.
useIsDarkTheme and useColorMapping hooks can be extracted for reuse.
| | 'donut' | ||
| | 'heatmap' | ||
| | 'histogram' | ||
| | 'polar'; |
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.
Can’t we use the same mapping as DeclarativeChart?
| const chartProps = transformer(spec, colorMap, isDarkTheme) as any; | ||
|
|
||
| // Special handling for charts with different prop patterns | ||
| if (chartType.type === 'donut') { |
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.
These conditions may not be needed since legendProps and componentRef are available on all charts.
| * @param encoding - Vega-Lite encoding with polar axis settings | ||
| * @returns Object with projected x, y arrays and metadata | ||
| */ | ||
| function projectPolarToCartesian( |
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.
projectPolarToCartesian, transformVegaLiteToPolarLineChartProps and transformVegaLiteToPolarScatterChartProps functions can be removed.
Previous Behavior
New Behavior
Related Issue(s)