Charting: Vertical stacked bar chart minor changes including uncaught type error issue fix. #15447

jameelakowsar · 2020-10-09T15:46:29Z

Pull request checklist

Addresses an existing issue: Fixes #Charting: Vertical stacked bar chart, on click of bars, 'uncaught type error' showing in consol. #15465
Include a change request file using $ yarn change

Description of changes

Vertical stacked bar chart.

'Uncaught type error' issue fixed when click event triggers on bars.
Adjusted margin props to the cartesian/specific charts.
Updated below conditions as like previous PR: Vertical stacked bar chart combined callout and other tweaks #14912
- Non zero bars condition added
- Changed 'data-is-focusable' param to undefined instead of false.
- Ref's data updated.
Margins condition updated. Now 0 value can be accepted.
Added description(default prop values) to the margins prop

Note: As Cartesian component consumed by other charts, Due to small change in cartesian, cross checked all other charts and added screenshots.

Focus areas to test

Vertical stacked bar chart
Vertical bar chart
Area chart
Line chart

After fix

margins: {
left: 45,
right: 30
}

Vertical stacked bar chart

Vertical stacked bar chart with RTL

margins: {{
left: 45,
right: 30
}}

Vertical bar chart

Vertical bar chart with RTL

No margins given - It can take default values `left:40, right: 20`

Area chart

Area chart with RTL

Line chart

Line chart with RTL

Test link: http://fabricweb.z5.web.core.windows.net/pr-deploy-site/refs/pull/15447/merge/charting/dist/index.html#/examples/VerticalStackedBarChart

codesandbox-ci · 2020-10-09T16:06:19Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3cc6329:

Sandbox	Source
Fluent UI Button	Configuration
codesandbox-react-template	Configuration
codesandbox-react-next-template	Configuration
codesandbox-react-northstar-template	Configuration

msft-github-bot · 2020-10-09T16:17:46Z

Perf Analysis

No significant results to display.

All results

Scenario	Render type	7.0 Ticks	PR Ticks	Iterations
Avatar	mount	855	874	5000
BaseButton	mount	931	893	5000
Breadcrumb	mount	42574	42579	5000
ButtonNext	mount	687	655	5000
Checkbox	mount	1629	1558	5000
CheckboxBase	mount	1359	1273	5000
ChoiceGroup	mount	5008	4996	5000
ComboBox	mount	964	966	1000
CommandBar	mount	7936	8073	1000
ContextualMenu	mount	15783	16581	1000
DefaultButton	mount	1137	1164	5000
DetailsRow	mount	3616	3710	5000
DetailsRowFast	mount	3869	3872	5000
DetailsRowNoStyles	mount	3432	3566	5000
Dialog	mount	1554	1518	1000
DocumentCardTitle	mount	1806	1899	1000
Dropdown	mount	2633	2657	5000
FocusTrapZone	mount	1735	1778	5000
FocusZone	mount	1851	1813	5000
IconButton	mount	1787	1782	5000
Label	mount	339	363	5000
Layer	mount	1972	1998	5000
Link	mount	485	460	5000
MenuButton	mount	1474	1461	5000
MessageBar	mount	2104	2111	5000
Nav	mount	3409	3371	1000
OverflowSet	mount	1457	1477	5000
Panel	mount	1483	1499	1000
Persona	mount	911	888	1000
Pivot	mount	1455	1424	1000
PrimaryButton	mount	1348	1290	5000
Rating	mount	7809	7747	5000
SearchBox	mount	1298	1283	5000
Shimmer	mount	2596	2682	5000
Slider	mount	1590	1619	5000
SpinButton	mount	5129	5191	5000
Spinner	mount	425	446	5000
SplitButton	mount	3202	3318	5000
Stack	mount	523	499	5000
StackWithIntrinsicChildren	mount	1634	1594	5000
StackWithTextChildren	mount	4789	4785	5000
SwatchColorPicker	mount	10653	10727	5000
TagPicker	mount	2764	2806	5000
TeachingBubble	mount	51721	51849	5000
Text	mount	457	440	5000
TextField	mount	1439	1491	5000
ThemeProvider	mount	1783	1722	5000
ThemeProvider	virtual-rerender	631	640	5000
Toggle	mount	882	867	5000
button	mount	129	123	5000

Perf Analysis (Fluent)

Perf comparison

Status	Scenario	Fluent TPI	Fabric TPI	Ratio	Iterations	Ticks
🎯	Avatar.Fluent	0.46	0.49	0.94:1	2000	911
🦄	Button.Fluent	0.12	0.2	0.6:1	5000	597
🔧	Checkbox.Fluent	0.69	0.35	1.97:1	1000	685
🎯	Dialog.Fluent	0.16	0.23	0.7:1	5000	820
🔧	Dropdown.Fluent	3.01	0.48	6.27:1	1000	3008
🔧	Icon.Fluent	0.14	0.06	2.33:1	5000	725
🦄	Image.Fluent	0.08	0.12	0.67:1	5000	404
🔧	Slider.Fluent	1.61	0.36	4.47:1	1000	1608
🔧	Text.Fluent	0.07	0.03	2.33:1	5000	371
🦄	Tooltip.Fluent	0.12	16.89	0.01:1	5000	575

🔧 Needs work 🎯 On target 🦄 Amazing

Perf tests with no regressions

Scenario	Current PR Ticks	Baseline Ticks	Ratio
AccordionMinimalPerf.default	161	0	Infinity:1
AlertMinimalPerf.default	311	0	Infinity:1
AnimationMinimalPerf.default	416	0	Infinity:1
AttachmentMinimalPerf.default	173	0	Infinity:1
AttachmentSlotsPerf.default	1201	0	Infinity:1
BoxMinimalPerf.default	391	0	Infinity:1
ButtonMinimalPerf.default	190	0	Infinity:1
ButtonOverridesMissPerf.default	1749	0	Infinity:1
ButtonSlotsPerf.default	611	0	Infinity:1
ButtonUseCssPerf.default	839	0	Infinity:1
ButtonUseCssNestingPerf.default	1132	0	Infinity:1
CardMinimalPerf.default	603	0	Infinity:1
CarouselMinimalPerf.default	483	0	Infinity:1
ChatDuplicateMessagesPerf.default	453	0	Infinity:1
ChatMinimalPerf.default	659	0	Infinity:1
ChatWithPopoverPerf.default	479	0	Infinity:1
CheckboxMinimalPerf.default	2945	0	Infinity:1
DialogMinimalPerf.default	843	0	Infinity:1
DividerMinimalPerf.default	414	0	Infinity:1
DropdownManyItemsPerf.default	780	0	Infinity:1
DropdownMinimalPerf.default	2978	0	Infinity:1
EmbedMinimalPerf.default	1985	0	Infinity:1
FlexMinimalPerf.default	329	0	Infinity:1
FormMinimalPerf.default	457	0	Infinity:1
GridMinimalPerf.default	375	0	Infinity:1
HeaderMinimalPerf.default	426	0	Infinity:1
HeaderSlotsPerf.default	827	0	Infinity:1
InputMinimalPerf.default	1412	0	Infinity:1
ItemLayoutMinimalPerf.default	1351	0	Infinity:1
LabelMinimalPerf.default	442	0	Infinity:1
LayoutMinimalPerf.default	470	0	Infinity:1
ListCommonPerf.default	700	0	Infinity:1
ListMinimalPerf.default	531	0	Infinity:1
ListNestedPerf.default	587	0	Infinity:1
ListWith60ListItems.default	924	0	Infinity:1
LoaderMinimalPerf.default	742	0	Infinity:1
MenuMinimalPerf.default	901	0	Infinity:1
MenuButtonMinimalPerf.default	1600	0	Infinity:1
PopupMinimalPerf.default	721	0	Infinity:1
PortalMinimalPerf.default	183	0	Infinity:1
ProviderMergeThemesPerf.default	2176	0	Infinity:1
ProviderMinimalPerf.default	1057	0	Infinity:1
RadioGroupMinimalPerf.default	477	0	Infinity:1
ReactionMinimalPerf.default	430	0	Infinity:1
RefMinimalPerf.default	257	0	Infinity:1
SegmentMinimalPerf.default	366	0	Infinity:1
SkeletonMinimalPerf.default	441	0	Infinity:1
SliderMinimalPerf.default	1593	0	Infinity:1
SplitButtonMinimalPerf.default	3894	0	Infinity:1
StatusMinimalPerf.default	729	0	Infinity:1
IconMinimalPerf.default	675	0	Infinity:1
TableManyItemsPerf.default	2283	0	Infinity:1
TableMinimalPerf.default	436	0	Infinity:1
TextMinimalPerf.default	372	0	Infinity:1
CustomToolbarPrototype.default	4004	0	Infinity:1
TooltipMinimalPerf.default	836	0	Infinity:1
TreeMinimalPerf.default	897	0	Infinity:1
TreeWith60ListItems.default	199	0	Infinity:1
VideoMinimalPerf.default	660	0	Infinity:1
Avatar.Fluent	911	0	Infinity:1
Button.Fluent	597	0	Infinity:1
Checkbox.Fluent	685	0	Infinity:1
Dialog.Fluent	820	0	Infinity:1
Dropdown.Fluent	3008	0	Infinity:1
Icon.Fluent	725	0	Infinity:1
Image.Fluent	404	0	Infinity:1
Slider.Fluent	1608	0	Infinity:1
Text.Fluent	371	0	Infinity:1
Tooltip.Fluent	575	0	Infinity:1
ToolbarMinimalPerf.default	991	1	991:1
TextAreaMinimalPerf.default	534	1	534:1
AvatarMinimalPerf.default	501	1	501:1
ImageMinimalPerf.default	401	1	401:1

size-auditor · 2020-10-09T16:24:45Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: c92c7c4caa7b3f97fc1b7198ec603bd6317ac851 (build)

mbest · 2020-10-09T17:04:03Z

@dzearing Why is the "Charting" label no longer applied automatically?

mbest · 2020-10-09T17:05:18Z

@jameelakowsar I will test these changes against our code and let know what works or still doesn't.

dzearing · 2020-10-09T18:10:03Z

@ecraig12345 I think the answer to @mbest question above is that the bot is perhaps looking only at target=master PRs? But i'm not sure.

ecraig12345 · 2020-10-09T18:27:17Z

Yeah it looks like the bot only considers master PRs for path-based labeling, and I don't see an option to consider other branches unfortunately.

ecraig12345 · 2020-10-09T18:28:49Z

Oh actually, I take that back. The way that the configuration works for this feature is a bit weird, and I think we need to update it to ignore changes to react-examples when considering what labels to use. I'll do that now.

mbest · 2020-10-11T21:57:31Z

I've verified that this fixes the following regressions:

script error from _redirectToUrl: Uncaught TypeError: Cannot read property 'props' of undefined
x-axis position ignores bottom margin (it's only in the right place when margin is 35px)

There are still these issues:

zero-value data isn't skipped
a zero right margin is ignored (defaults to 20px)
left margin of x axis is always 40px (ignores provided margin; the length of the axis lines is correct though, just the position is wrong)

mbest · 2020-10-11T21:58:16Z

As shown above, the translate value should match the left margin, but is always 40 instead.

mbest · 2020-10-11T22:08:52Z

In RTL mode, the right margin isn't handled correctly. Comparing the svg width to the second translate value, they should be different by the amount of the right margin (36px in this case), but instead they are 72px different.

Here's the same chart before the changes in #15061

jameelakowsar · 2020-10-12T08:48:44Z

I've verified that this fixes the following regressions:

script error from _redirectToUrl: Uncaught TypeError: Cannot read property 'props' of undefined

x-axis position ignores bottom margin (it's only in the right place when margin is 35px)

There are still these issues:

zero-value data isn't skipped

a zero right margin is ignored (defaults to 20px)

left margin of x axis is always 40px (ignores provided margin; the length of the axis lines is correct though, just the position is wrong)

What meant by 'zero-value' data?
For remaining, Please check email once.
Test link provided in PR description.

jameelakowsar · 2020-10-12T13:36:00Z

@mbest ,

If all good, I will make this PR to open. Confirm your status.

packages/charting/src/components/CommonComponents/CartesianChart.types.ts

...act-examples/src/charting/VerticalStackedBarChart/VerticalStackedBarChart.Styled.Example.tsx

packages/charting/src/components/CommonComponents/CartesianChart.base.tsx

mbest · 2020-10-13T00:04:18Z

@jameelakowsar

What meant by 'zero-value' data?

Before your "refactor" I had added this line to filter our zero-value data:

    const nonZeroBars = singleChartData.chartData.filter(point => point.data > 0);

I see from looking through the _createBar function that you undid most of the improvements I made, including that one. Additionally, it's back to using refArrayIndexNumber which is broken if the stacks don't all have the same number of data items. You also undid some code cleanup I did. For example I had:

    const xPoint = xBarScale(isNumeric ? (singleChartData.xAxisPoint as number) : indexNumber);

which you changed back to (and moved back into the loop even though it doesn't need to be there):

        let xPoint;
        if (this._isNumeric) {
          xPoint = xBarScale(singleChartData.xAxisPoint as number);
        } else {
          xPoint = xBarScale(indexNumber);
        }

You also undid the normalization of the props that are passed to the callout handlers.

You also undid the code that makes sure that data-is-focusable isn't set to a value of false, which breaks focus issues.

msft-github-bot · 2020-10-15T12:23:38Z

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

msft-github-bot · 2020-10-15T12:23:41Z

🎉@fluentui/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

…ade minor edits to margins (#15699) Cherry-pick of #15447.

…ade minor edits to margins (microsoft#15699) Cherry-pick of microsoft#15447.

Jameela Kowsar Shaik (Zen3 Infosolutions America Inc) added 3 commits October 9, 2020 20:58

Vertical stacked bar chart minor edits

18f155f

Change files

303aec2

change files

77abee4

msft-github-bot added the Fabric 7 label Oct 9, 2020

msft-github-bot assigned tomi-msft Oct 9, 2020

msft-github-bot added the needs cherry-pick Temporary label for PRs which may need to be cherry-picked to master label Oct 9, 2020

Raghurk approved these changes Oct 9, 2020

View reviewed changes

khmakoto mentioned this pull request Oct 9, 2020

PRs targeting the 7.0 branch that will need porting into master #15285

Closed

40 tasks

khmakoto added the Package: charting label Oct 9, 2020

Ridvanovskyy approved these changes Oct 9, 2020

View reviewed changes

margins default description added and minor changes to cartesian margins

b67c02f

lint warnings

c5ec75f

ecraig12345 reviewed Oct 12, 2020

View reviewed changes

packages/charting/src/components/CommonComponents/CartesianChart.types.ts Outdated Show resolved Hide resolved

...act-examples/src/charting/VerticalStackedBarChart/VerticalStackedBarChart.Styled.Example.tsx Outdated Show resolved Hide resolved

mbest suggested changes Oct 12, 2020

View reviewed changes

packages/charting/src/components/CommonComponents/CartesianChart.base.tsx Outdated Show resolved Hide resolved

msft-github-bot added the Needs: Author Feedback label Oct 12, 2020

Non zero data, ref values updated

3cc6329

msft-github-bot removed the Needs: Author Feedback label Oct 13, 2020

jameelakowsar marked this pull request as ready for review October 14, 2020 12:02

Raghurk merged commit 8828fdc into microsoft:7.0 Oct 14, 2020

khmakoto mentioned this pull request Oct 26, 2020

Charting: Fixing uncaught type error in VerticalStackedBarChart and made minor edits to margins #15699

Merged

2 tasks

ecraig12345 pushed a commit that referenced this pull request Oct 26, 2020

Charting: Fixing uncaught type error in VerticalStackedBarChart and m…

26ce5d6

…ade minor edits to margins (#15699) Cherry-pick of #15447.

SethDonohue pushed a commit to SethDonohue/fluentui that referenced this pull request Nov 2, 2020

Charting: Fixing uncaught type error in VerticalStackedBarChart and m…

640d076

…ade minor edits to margins (microsoft#15699) Cherry-pick of microsoft#15447.

ecraig12345 removed the needs cherry-pick Temporary label for PRs which may need to be cherry-picked to master label Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Charting: Vertical stacked bar chart minor changes including uncaught type error issue fix. #15447

Charting: Vertical stacked bar chart minor changes including uncaught type error issue fix. #15447

jameelakowsar commented Oct 9, 2020 •

edited

Loading

codesandbox-ci bot commented Oct 9, 2020 •

edited

Loading

msft-github-bot commented Oct 9, 2020 •

edited

Loading

size-auditor bot commented Oct 9, 2020 •

edited

Loading

mbest commented Oct 9, 2020

mbest commented Oct 9, 2020

dzearing commented Oct 9, 2020

ecraig12345 commented Oct 9, 2020

ecraig12345 commented Oct 9, 2020

mbest commented Oct 11, 2020

mbest commented Oct 11, 2020

mbest commented Oct 11, 2020 •

edited

Loading

jameelakowsar commented Oct 12, 2020

jameelakowsar commented Oct 12, 2020

mbest commented Oct 13, 2020 •

edited

Loading

msft-github-bot commented Oct 15, 2020

msft-github-bot commented Oct 15, 2020

Charting: Vertical stacked bar chart minor changes including uncaught type error issue fix. #15447

Charting: Vertical stacked bar chart minor changes including uncaught type error issue fix. #15447

Conversation

jameelakowsar commented Oct 9, 2020 • edited Loading

Pull request checklist

Description of changes

Focus areas to test

After fix

Vertical stacked bar chart

Vertical stacked bar chart with RTL

Vertical bar chart

Vertical bar chart with RTL

No margins given - It can take default values left:40, right: 20

Area chart

Area chart with RTL

Line chart

Line chart with RTL

Test link: http://fabricweb.z5.web.core.windows.net/pr-deploy-site/refs/pull/15447/merge/charting/dist/index.html#/examples/VerticalStackedBarChart

codesandbox-ci bot commented Oct 9, 2020 • edited Loading

msft-github-bot commented Oct 9, 2020 • edited Loading

Perf Analysis

Perf Analysis (Fluent)

size-auditor bot commented Oct 9, 2020 • edited Loading

Asset size changes

mbest commented Oct 9, 2020

mbest commented Oct 9, 2020

dzearing commented Oct 9, 2020

ecraig12345 commented Oct 9, 2020

ecraig12345 commented Oct 9, 2020

mbest commented Oct 11, 2020

mbest commented Oct 11, 2020

mbest commented Oct 11, 2020 • edited Loading

jameelakowsar commented Oct 12, 2020

jameelakowsar commented Oct 12, 2020

mbest commented Oct 13, 2020 • edited Loading

msft-github-bot commented Oct 15, 2020

msft-github-bot commented Oct 15, 2020

jameelakowsar commented Oct 9, 2020 •

edited

Loading

No margins given - It can take default values `left:40, right: 20`

codesandbox-ci bot commented Oct 9, 2020 •

edited

Loading

msft-github-bot commented Oct 9, 2020 •

edited

Loading

size-auditor bot commented Oct 9, 2020 •

edited

Loading

mbest commented Oct 11, 2020 •

edited

Loading

mbest commented Oct 13, 2020 •

edited

Loading