Skip to content

Conversation

@AtishayMsft
Copy link
Contributor

Previous Behavior

New Behavior

Related Issue(s)

  • Fixes #

@github-actions
Copy link

github-actions bot commented Dec 6, 2025

📊 Bundle size report

✅ No changes found

@github-actions
Copy link

github-actions bot commented Dec 6, 2025

Pull request demo site: URL

@@ -0,0 +1,247 @@
# Vega-Lite Schema Integration Summary
Copy link

@github-actions github-actions bot Dec 6, 2025

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.

atisjai and others added 15 commits January 7, 2026 07:41
@AtishayMsft AtishayMsft marked this pull request as ready for review January 30, 2026 08:30
@AtishayMsft AtishayMsft requested review from a team as code owners January 30, 2026 08:30
Copy link
Contributor

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 {
Copy link
Contributor

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:

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;
}

Copy link
Contributor

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: {
Copy link
Contributor

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',
Copy link
Contributor

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 = [
Copy link
Contributor

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')
Copy link
Contributor

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))

Copy link
Contributor

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 {
Copy link
Contributor

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';
Copy link
Contributor

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') {
Copy link
Contributor

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(
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants