Skip to content
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(declarative-chart): Create Scatter plot #33843

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Anush2303
Copy link
Contributor

@Anush2303 Anush2303 commented Feb 14, 2025

New Behavior

Created Scatter plot from points in line chart.
image

image

Examples generated using BizChat
image

image

Related Issue(s)

  • Fixes #

Copy link

github-actions bot commented Feb 14, 2025

📊 Bundle size report

✅ No changes found

@@ -225,17 +225,21 @@ export const DeclarativeChart: React.FunctionComponent<DeclarativeChartProps> =
);
Copy link
Collaborator

@fabricteam fabricteam Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕵🏾‍♀️ visual regressions to review in the fluentuiv8 Visual Regression Report

Callout 1 screenshots
Image Name Diff(in Pixels) Image Type
Callout.No callout width specified.default.chromium.png 2319 Changed
react-charting-HeatMapChart 1 screenshots
Image Name Diff(in Pixels) Image Type
react-charting-HeatMapChart.Basic - RTL.default.chromium.png 372 Changed
react-charting-LineChart 1 screenshots
Image Name Diff(in Pixels) Image Type
react-charting-LineChart.Gaps.default.chromium.png 1 Changed

Copy link

Pull request demo site: URL

@Anush2303 Anush2303 marked this pull request as ready for review February 17, 2025 06:37
@Anush2303 Anush2303 requested a review from a team as a code owner February 17, 2025 06:37
@@ -43,6 +43,7 @@ import {
} from '../index';
import { formatPrefix as d3FormatPrefix } from 'd3-format';

let maxMarkerSize = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxMarkerSize

why using a global variable? The utilities file is supposed to be stateless

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree with this. But maxMarkerSize is required at other place(createNumericYAxis) also in Utilites file where we don't have access to the line chart points from where we are calculating this. That is why I added this as global variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. The value could be changed by another caller of the utility. This should be part of the class component only.

: 0;
const domainValues = prepareDatapoints(
finalYmax + maxMarkerSizeInPixelsForYAxis,
finalYmin - maxMarkerSizeInPixelsForYAxis,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define a new function called updateDomainValues

Copy link
Contributor Author

@Anush2303 Anush2303 Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we define this? maxMarkerSizeInPixelsForYAxis is by default 0 only. The existing logic will remain intact.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't be needed if the marker specific logic moves to the lineChart.base file.

@AtishayMsft
Copy link
Contributor

AtishayMsft commented Feb 19, 2025

export function domainRangeOfNumericForAreaChart(

define an additional argument called padding to utility functions. And move the marker size logic to line chart only


Refers to: packages/charts/react-charting/src/utilities/utilities.ts:906 in a739a12. [](commit_id = a739a12, deletion_comment = False)

});
})!;
const maxMarkerSizeForXAxis = maxMarkerSize
? Math.abs((maxMarkerSize * (xMax - xMin)) / (width - margins.right! - margins.left! - 2 * maxMarkerSize))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the equation is same in both cases: $x = y * {x2 - x1 \over y2 - y1 - 2*y}$
The only difference is that the sign of $y$ depends on the condition $y2 &gt; y1$.

You can extract this equation into a separate function so that if we need to extend the domain of a linear scale: $[x1, x2] \to [y1, y2]$ to accommodate additional $y$ pixels, we can simply call this function without having to rederive the equation every time.

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.

4 participants