-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(react-charting): Adding Scatter Chart component #34374
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-charting): Adding Scatter Chart component #34374
Conversation
📊 Bundle size report✅ No changes found |
change/@fluentui-react-charting-d0567e03-b642-45d3-b66c-294429154795.json
Show resolved
Hide resolved
Pull request demo site: URL |
packages/charts/react-charting/src/components/ScatterChart/ScatterChart.base.tsx
Show resolved
Hide resolved
packages/charts/react-charting/src/components/ScatterChart/ScatterChart.base.tsx
Show resolved
Hide resolved
packages/charts/react-charting/src/components/ScatterChart/ScatterChart.base.tsx
Show resolved
Hide resolved
@@ -157,11 +157,7 @@ const validateBarData = (data: Partial<PlotData>) => { | |||
}; | |||
|
|||
const validateScatterData = (data: Partial<PlotData>) => { | |||
if (['markers', 'text+markers', 'markers+text'].includes(data.mode ?? '') && !isNumberArray(data.x)) { |
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.
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.
removed 'markers' and kept the rest
/** | ||
* {@docCategory IChartData} | ||
*/ | ||
export interface IBaseDataPoint { |
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.
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.
the order of declaring interfaces in TypeScript does not matter as far as I know
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.
the order does not matter for functionality but helps to maintain readability as this file has become huge.
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.
updated
@@ -167,10 +169,11 @@ export const DeclarativeChart: React.FunctionComponent<DeclarativeChartProps> = | |||
|
|||
const renderLineArea = (plotlyData: Data[], isAreaChart: boolean): JSX.Element => { | |||
const isScatterMarkers = ['markers', 'text+markers', 'markers+text'].includes((plotlyData[0] as PlotData)?.mode); | |||
const chartProps: ILineChartProps | IAreaChartProps = { | |||
const chartType = isAreaChart ? 'Area' : isScatterMarkers ? 'Scatter' : 'Line'; |
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.
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.
any reason for making it small case?
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.
in general fluent follows snakeCase for string literals. I meant first letter smallcase
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.
updated
@@ -579,9 +587,8 @@ export const transformPlotlyJsonToScatterChartProps = ( | |||
height: input.layout?.height ?? 350, | |||
hideTickOverlap: true, | |||
enableReflow: false, | |||
hideLegend, |
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.
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.
done
React.useImperativeHandle( | ||
props.componentRef, | ||
() => ({ | ||
chartContainer: cartesianChartRef.current?.chartContainer ?? null, |
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.
is export as image working correctly
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.
yes
setIsSelectedLegend(false); | ||
} | ||
|
||
function _createLegends(data: ScatterChartDataWithIndex[]): JSX.Element { |
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.
is editing of legends from textbox and updating to textbox working as expected
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.
yes all are working as expected
@@ -180,6 +183,9 @@ export const DeclarativeChart: React.FunctionComponent<DeclarativeChartProps> = | |||
if (isAreaChart) { | |||
return <ResponsiveAreaChart {...chartProps} />; | |||
} | |||
if (isScatterMarkers) { | |||
return <ResponsiveScatterChart {...(chartProps as IScatterChartProps)} />; | |||
} | |||
return <ResponsiveLineChart {...chartProps} lineMode={isScatterMarkers ? 'scatter' : 'default'} />; |
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.
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.
done
For XYear for scatter, don't fallback to VSBC as year is rendered as categorical/string. Refers to: packages/charts/react-charting/src/components/DeclarativeChart/DeclarativeChart.tsx:205 in a0d82be. [](commit_id = a0d82be, deletion_comment = False) |
I have already added an example for String x-axis in react-eamples, also there is a screenshot for it in description |
Ok. Then is test schema 433 working now. |
yes displaying as Scatter chart |
Updated playwright report: https://github.com/microsoft/fluentui-charting-contrib/actions/runs/15413440080 |
Adding Scatter Chart component in V8.
Also enabling year as categorical value for scatter chart.
Work items:
Basic:

Date Axis:

String Axis:
