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

fix(react-charting): declarative chart bug fixes #33567

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

fix(react-charting): declarative chart bug fixes #33567

wants to merge 9 commits into from
+335 −141

Conversation

Anush2303
Copy link
Contributor

@Anush2303 Anush2303 commented Jan 7, 2025 •
edited
Loading

Previous Behavior

New Behavior

Following fixes are made:

  1. Legend and chart greying out for single legend case in Area chart
  2. Order of Y bars inverted in HBC with axis.
  3. Labels not coming in percent in pie chart.
  4. Support for correct date values.
  5. Replaced gauge indicator plotly color with Semantic colors.

Related Issue(s)

  • Fixes #

@Anush2303 Anush2303 marked this pull request as ready for review January 7, 2025 08:01
@Anush2303 Anush2303 requested a review from a team as a code owner January 7, 2025 08:01
@github-actions GitHub Actions
Copy link

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

📊 Bundle size report

✅ No changes found

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 7, 2025

Pull request demo site: URL

srmukher
srmukher reviewed Jan 8, 2025
View reviewed changes
packages/charts/react-charting/src/components/AreaChart/AreaChart.base.tsx
@@ -450,7 +450,7 @@ export class AreaChartBase extends React.Component<IAreaChartProps, IAreaChartSt
});
stackedData.push(currentStack);
});
this._isMultiStackChart = stackedData && stackedData.length > 1 ? true : false;
this._isMultiStackChart = stackedData && stackedData.length >= 1 ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

if stackedData length == 1, then is it multi-stacked?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Logically not. But _isMultiStackChart reference is used only to set the opacity of the chart.
image
To grey out chart in case of single legend also, it has to fail the first if condition.

Copy link
Contributor

Choose a reason for hiding this comment

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

in what case will this value be 0?

Copy link
Contributor Author

@Anush2303 Anush2303 Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I agree, it won't be zero. Then what should we do, either we can ommit this property altogether? Because without changing this, the chart will not grey out. And this is used only to set the opacity.

srmukher
srmukher reviewed Jan 9, 2025
View reviewed changes
packages/charts/react-charting/src/components/DeclarativeChart/PlotlySchemaAdapter.ts
}
const parsedYear = parsedDate.getFullYear();
const yearInString = /\b\d{4}\b/.test(value);
if (!yearInString && (parsedYear === 2000 || parsedYear === 2001)) {
Copy link
Contributor

@srmukher srmukher Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Found that parsed year can have values other than 2000 and 2001 also. And in those cases, isDate() is also returning true:

Value : Year Returned by parsedYear(Value) : isDate(Value)
-1 to -12 : 2001 : true
-13 to -31 : no year : false
-32 to -49 : 2032 to 2049 : true
-50 to -99 : 1950 to 1999 : true
0 : 2000 : true
1 to 12 : 2001 : true
13 to 31 : no year : false
32 to 49 : 2032 - 2049 : true
50 to 99 : 1950 - 1999 : true

Are these expected?

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, this is expected.

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

@srmukher srmukher srmukher left review comments

@AtishayMsft AtishayMsft AtishayMsft left review comments

@fabricteam fabricteam fabricteam 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.
4 participants
@Anush2303 @fabricteam @AtishayMsft @srmukher