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.

Sign up for GitHub

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

Jump to bottom

Enable 'tozeroy' mode for Area Charts #33581

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

Enable 'tozeroy' mode for Area Charts #33581

wants to merge 7 commits into from

Conversation

srmukher
Copy link
Contributor

@srmukher srmukher commented Jan 8, 2025

Enable 'tozeroy' mode for Area Charts

image

@srmukher srmukher requested a review from a team as a code owner January 8, 2025 10:24
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 8, 2025 •
edited
Loading

📊 Bundle size report

✅ No changes found

@srmukher srmukher force-pushed the users/srmukher/zero_to_y branch from f54f284 to 43263f2 Compare January 8, 2025 11:25
@srmukher srmukher force-pushed the users/srmukher/zero_to_y branch 2 times, most recently from 75a3500 to 7fbe2c1 Compare January 9, 2025 06:43
@srmukher srmukher force-pushed the users/srmukher/zero_to_y branch from 7fbe2c1 to 561c69e Compare January 9, 2025 07:46
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 9, 2025

Pull request demo site: URL

AtishayMsft
AtishayMsft reviewed Jan 9, 2025
View reviewed changes
packages/charts/react-charting/src/components/AreaChart/AreaChart.types.ts Outdated
/**
* The prop used to define the Y axis mode (tonexty or tozeroy)
*/
mode?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

mode

use mode: 'tonexty'|'tozeroy'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@AtishayMsft AtishayMsft Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

dont see this changed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see the change:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

not in any commit here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its in commit "Merging functions and renaming variables". Check the AreaChart.types.ts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I get it. the enum is not removed. You are still using the enum in the base.tsx file

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, will keep it as modes can extend to "values": [
"none",
"tozeroy",
"tozerox",
"tonexty",
"tonextx",
"toself",
"tonext"
], as per the plotly schema

Copy link
Contributor

Choose a reason for hiding this comment

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

same functionality can be achieved using string enums also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok changing it

AtishayMsft
AtishayMsft reviewed Jan 10, 2025
View reviewed changes
packages/charts/react-charting/src/components/AreaChart/AreaChart.base.tsx Outdated
private _getData = (keys: string[], dataSet: any) => {
const dataValues = d3Stack().keys(keys)(dataSet);
const maxOfYVal = d3Max(dataValues[dataValues.length - 1], dp => dp[1])!;
const renderData: Array<IAreaChartDataSetPoint[]> = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

renderData

renderPoints

AtishayMsft
AtishayMsft reviewed Jan 10, 2025
View reviewed changes
packages/charts/react-charting/src/components/AreaChart/AreaChart.base.tsx Outdated
currentStack.push({
values: d,
currentLayer.push({
values: this.props.mode === AreaChartModes.toZeroY ? [0, d[1]] : d,
Copy link
Contributor

Choose a reason for hiding this comment

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

add a snapshot test for this change

AtishayMsft
AtishayMsft reviewed Jan 10, 2025
View reviewed changes
packages/charts/react-charting/src/components/AreaChart/AreaChart.base.tsx Outdated
const stackedValues = d3Stack().keys(keys)(dataSet);
const maxOfYVal = d3Max(stackedValues[stackedValues.length - 1], dp => dp[1])!;
const stackedData: Array<IAreaChartDataSetPoint[]> = [];
private _getData = (keys: string[], dataSet: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

_getData

rename to _getDataPoints

AtishayMsft
AtishayMsft reviewed Jan 10, 2025
View reviewed changes
packages/charts/react-charting/src/components/AreaChart/AreaChart.base.tsx
const maxOfYVal = d3Max(stackedValues[stackedValues.length - 1], dp => dp[1])!;
const stackedData: Array<IAreaChartDataSetPoint[]> = [];
private _getData = (keys: string[], dataSet: any) => {
const dataValues = d3Stack().keys(keys)(dataSet);
Copy link
Contributor

Choose a reason for hiding this comment

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

d3Stack

do we still need to use d3 stack for toZeroY mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

without d3Stack, the chart gets distorted:
image

@AtishayMsft
Copy link
Contributor

Enable 'tozeroy' mode for Area Charts

image

The overlapping color is not coming correctly.

@srmukher
Copy link
Contributor Author

srmukher commented Jan 10, 2025 •
edited
Loading

Enable 'tozeroy' mode for Area Charts
image

The overlapping color is not coming correctly.

This is because the layers have different colors and one over the other. Clicking each legend individually gives the individual colors correctly. Similar is for plotly:
image

@AtishayMsft
Copy link
Contributor

Enable 'tozeroy' mode for Area Charts
image

The overlapping color is not coming correctly.

This is because the layers have different colors and one over the other. Clicking each legend individually gives the individual colors correctly. Similar is for plotly: image

this needs to be fixed

@fabricteam
Copy link
Collaborator

🕵 fluentuiv8 No visual regressions between this PR and main

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

@AtishayMsft AtishayMsft AtishayMsft left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@srmukher @AtishayMsft @fabricteam