[Donut] Legends multi selection for Donut Charts #33447

srmukher · 2024-12-11T11:37:04Z

Legends multi selection for Donut Charts

github-actions · 2024-12-11T11:39:47Z

📊 Bundle size report

✅ No changes found

github-actions · 2024-12-11T11:46:01Z

Pull request demo site: URL

packages/charts/react-charting/src/components/DonutChart/Arc/Arc.tsx

AtishayMsft · 2024-12-12T08:12:57Z

packages/react-examples/src/react-charting/DonutChart/DonutChart.Multiple.Example.tsx

+  roundCorners: boolean;
+}
+
+export class DonutChartMultipleExample extends React.Component<IDonutChartProps, IDonutChartState> {


DonutChartMultipleExample

don't create a new example. Add a toggle in the existing example.

AtishayMsft · 2024-12-12T08:17:43Z

packages/charts/react-charting/src/components/DonutChart/Arc/Arc.tsx

-    const opacity: number =
-      this.props.activeArc === this.props.data!.data.legend || this.props.activeArc === '' ? 1 : 0.1;
-
+    const isActive = activeArc?.includes(this.props.data?.data.legend!);


const isActive

why repeating this logic.

so that it can be used to calculate both opacity and data-is-focusable without the need to add same checks again

AtishayMsft · 2024-12-12T08:21:06Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

@@ -354,7 +374,7 @@ export class DonutChartBase extends React.Component<IDonutChartProps, IDonutChar
   * Note: This won't work in case of multiple legends selection.
   */
  private _getHighlightedLegend() {
-    return this.state.selectedLegend || this.state.activeLegend;


return this.state.selectedLegend || this.state.activeLegend;

This is an or condition between selected and active.

For multiple select also checking if this.state.selectedLegends is populated other return the this.state.activeLegend
return this.state.selectedLegends.length > 0 ? this.state.selectedLegends : [this.state.activeLegend];

AtishayMsft · 2024-12-12T08:23:03Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

-        if (point.legend === highlightedLegend || (this.state.showHover && point.legend === this.state.legend)) {
+        if (
+          highlightedLegends.includes(point.legend!) ||
+          (this.state.showHover && point.legend === this.state.legend)


point.legend === this.state.legend

you are adding selected legend and hover legend values together?

separated the logic

…icrosoft/fluentui into users/srmukher/LegendsMultiSel

AtishayMsft · 2024-12-16T11:30:18Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

@@ -265,11 +263,17 @@ export class DonutChartBase extends React.Component<IDonutChartProps, IDonutChar
        focusZonePropsInHoverCard={this.props.focusZonePropsForLegendsInHoverCard}
        overflowText={this.props.legendsOverflowText}
        {...this.props.legendProps}
+        canSelectMultipleLegends={this.props.canSelectMultipleLegends}
+        onChange={this._onLegendChange}


onChange={this._onLegendChange}

keep using the action event handler. As this can be overridden for a specific legend as per the caller.

Dont modify existing behavior.

modified as discussed offline

AtishayMsft · 2024-12-16T11:31:16Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.types.ts

+   * Prop to enable the multiple legend selection
+   * @default false
+   */
+  canSelectMultipleLegends?: boolean;


canSelectMultipleLegends?: boolean;

not needed. This is an existing property of the legends control

AtishayMsft · 2024-12-16T11:34:11Z

packages/charts/react-charting/src/components/DonutChart/Arc/Arc.tsx

-      this.props.activeArc === this.props.data!.data.legend || this.props.activeArc === '' ? 1 : 0.1;
-
+    const isActive = activeArc?.includes(this.props.data?.data.legend!);
+    const opacity: number = activeArc && activeArc.length > 0 ? (isActive ? 1 : 0.1) : 1;


activeArc && activeArc.length > 0 ? (isActive ? 1 : 0.1) : 1;

move this logic to isActive

AtishayMsft · 2024-12-16T11:35:36Z

packages/charts/react-charting/src/components/DonutChart/Arc/Arc.tsx

@@ -129,7 +131,8 @@ export class Arc extends React.Component<IArcProps, IArcState> {
    if (
      hideLabels ||
      Math.abs(data!.endAngle - data!.startAngle) < Math.PI / 12 ||
-      (activeArc !== data!.data.legend && activeArc !== '')
+      !Array.isArray(activeArc) ||
+      (data!.data.legend && activeArc.includes(data!.data.legend))


same logic is repeating

moved to a function

AtishayMsft · 2024-12-16T11:37:18Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

@@ -354,7 +365,7 @@ export class DonutChartBase extends React.Component<IDonutChartProps, IDonutChar
   * Note: This won't work in case of multiple legends selection.
   */
  private _getHighlightedLegend() {


_getHighlightedLegend

modify this function for multiselect

this was already modified

…sers/srmukher/LegendsMultiSel

AtishayMsft · 2024-12-18T05:33:18Z

packages/charts/react-charting/etc/react-charting.api.md

@@ -488,6 +488,7 @@ export interface IDonutChart {
 // @public
 export interface IDonutChartProps extends ICartesianChartProps {
    calloutProps?: Partial<ICalloutProps>;
+    canSelectMultipleLegends?: boolean;


canSelectMultipleLegends

revert this change

AtishayMsft · 2024-12-18T08:41:27Z

packages/charts/react-charting/src/components/DonutChart/Arc/Arc.tsx

@@ -123,13 +124,17 @@ export class Arc extends React.Component<IArcProps, IArcState> {
    return point.callOutAccessibilityData?.ariaLabel || (legend ? `${legend}, ` : '') + `${yValue}.`;
  };

+  private _shouldHighlightArc = (activeArc: string[], legend?: string): boolean => {


_shouldHighlightArc

this same function can serve all 3 places.

_shouldHighlightArc has already been used in other places too, any other specific places ?

AtishayMsft · 2024-12-18T08:43:39Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

@@ -24,9 +24,10 @@ export interface IDonutChartState {
  xCalloutValue?: string;
  yCalloutValue?: string;
  focusedArcId?: string;
-  selectedLegend: string;
+  selectedLegend?: string;


selectedLegend

Selected legend is not being used anywhere

AtishayMsft · 2024-12-18T08:44:49Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

@@ -138,7 +140,9 @@ export class DonutChartBase extends React.Component<IDonutChartProps, IDonutChar
                hoverLeaveCallback={this._hoverLeave}
                uniqText={this._uniqText}
                onBlurCallback={this._onBlur}
-                activeArc={this._getHighlightedLegend()}
+                activeArc={this._getHighlightedLegend().filter(


_getHighlightedLegend

use isLegendHighlighted here

AtishayMsft · 2024-12-18T08:45:24Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

        },
        hoverAction: () => {
          this._handleChartMouseLeave();
          this.setState({ activeLegend: point.legend! });
        },
        onMouseOutAction: () => {
-          this.setState({ activeLegend: '' });
+          this.setState({ activeLegend: undefined });


undefined

setting as empty should be ok here

no, checked with a legend having '' title, it does not work in that case

AtishayMsft · 2024-12-18T08:47:35Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

+  };
+
+  private _isLegendHovered = (legend: string): boolean => {
+    return this._noLegendsHighlighted() || this._isLegendHighlighted(legend);


this._noLegendsHighlighted()

this is redundant

keeping this for now as we will take up the change in experience task later.

AtishayMsft · 2024-12-18T08:49:25Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

@@ -354,7 +387,7 @@ export class DonutChartBase extends React.Component<IDonutChartProps, IDonutChar
   * Note: This won't work in case of multiple legends selection.
   */
  private _getHighlightedLegend() {
-    return this.state.selectedLegend || this.state.activeLegend;
+    return this.state.selectedLegends.length > 0 ? this.state.selectedLegends : [this.state.activeLegend];


return this.state.selectedLegends.length > 0 ? this.state.selectedLegends : [this.state.activeLegend];

this wont work if 2 legend are selected and a third legend is hovered

But, in line multiple chart, on multiple legend selection, legend hover is disabled. Shall the experience be not the same?

Yes. We will need a different representation. Currently hover behavior is lost once one or more legend is selected. We will plan for it. You can continue with the current behavior for now.

AtishayMsft · 2024-12-20T10:03:28Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

@@ -265,16 +259,41 @@ export class DonutChartBase extends React.Component<IDonutChartProps, IDonutChar
        focusZonePropsInHoverCard={this.props.focusZonePropsForLegendsInHoverCard}
        overflowText={this.props.legendsOverflowText}
        {...this.props.legendProps}
+        // eslint-disable-next-line react/jsx-no-bind
+        onChange={this._onLegendSelectionChange.bind(this)}


this._onLegendSelectionChange.bind(this)

do you need to use this twice?

This is used just once

i mean the class instance "this"

AtishayMsft · 2024-12-20T10:05:42Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

+    if (this.props.legendProps?.canSelectMultipleLegends) {
+      this.setState({ selectedLegends });
+    } else {
+      this.setState({ selectedLegends: selectedLegends.slice(-1) });


selectedLegends.slice(-1)

does this work when the legend is unselected

yes working

AtishayMsft · 2024-12-20T10:08:19Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

+
+  private _noLegendsHighlighted = (): boolean => {
+    return this._getHighlightedLegend().length === 0;
+  };


keep these functions together with getHighlightedLegends

AtishayMsft · 2024-12-20T10:10:59Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

  private _focusCallback = (data: IChartDataPoint, id: string, element: SVGPathElement): void => {
    this._currentHoverElement = element;
    this.setState({
      /** Show the callout if highlighted arc is focused and Hide it if unhighlighted arc is focused */
-      showHover: this.state.selectedLegend === '' || this.state.selectedLegend === data.legend,
+      showHover: this._noLegendsHighlighted() || this._isLegendHighlighted(data.legend!),


!

move this ! check inside isLegendHighlighted function

AtishayMsft · 2024-12-20T10:16:11Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

+    const highlightedLegends = this._getHighlightedLegend().filter(legend => legend !== undefined);
+
+    if (this.state.showHover) {
+      const legendValue = data.find(point => point.legend === this.state.legend);


legendValue

call this variable point

calling it pointValue as point is used in data.find

AtishayMsft · 2024-12-20T10:18:06Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

+  }
+
+  private _isLegendHighlighted = (legend: string): boolean => {
+    return this._getHighlightedLegend().indexOf(legend) > -1;


.indexOf(legend) > -1;

use .includes?

AtishayMsft · 2024-12-20T10:36:53Z

packages/charts/react-charting/src/components/DonutChart/Arc/Arc.tsx

@@ -123,13 +124,16 @@ export class Arc extends React.Component<IArcProps, IArcState> {
    return point.callOutAccessibilityData?.ariaLabel || (legend ? `${legend}, ` : '') + `${yValue}.`;
  };

+  private _shouldHighlightArc = (activeArc: string[], legend?: string): boolean => {


activeArc

activeArc can come from this.props. Not needed as argument. All the empty and length checks can move to this. Right?

AtishayMsft · 2024-12-20T11:18:35Z

packages/charts/react-charting/src/components/DonutChart/__snapshots__/DonutChart.test.tsx.snap

@@ -2443,22 +2429,6 @@ exports[`DonutChart snapShot testing Should render arc labels in percentage form
              onMouseOver={[Function]}
              role="img"
            />
-            <text


text

why this change

updated snapshots

AtishayMsft · 2024-12-20T11:18:57Z

...es/charts/react-charting/src/components/DonutChart/__snapshots__/DonutChartRTL.test.tsx.snap

-  </div>
-</div>
-`;
+exports[`Donut chart interactions Should reflect theme change 1`] = `<div />`;


<div />;

why did the div disappear

updated snapshots

…icrosoft/fluentui into users/srmukher/LegendsMultiSel

AtishayMsft · 2024-12-23T13:02:26Z

packages/charts/react-charting/src/components/DonutChart/Arc/Arc.tsx

-    const { arc, data, innerRadius, outerRadius, showLabelsInPercent, totalValue, hideLabels, activeArc } = this.props;
+  private _shouldHighlightArc = (legend?: string): boolean => {
+    const { activeArc } = this.props;
+    return !(activeArc && activeArc.length > 0 && legend !== undefined && !activeArc.includes(legend));


!

why is there negation sign here

AtishayMsft · 2024-12-23T13:03:27Z

packages/charts/react-charting/src/components/DeclarativeChart/PlotlySchemaAdapter.ts

@@ -94,6 +94,9 @@ export const transformPlotlyJsonToDonutProps = (
    hideLabels,
    showLabelsInPercent: firstData.textinfo ? firstData.textinfo === 'percent' : true,
    styles,
+    legendProps: {
+      canSelectMultipleLegends: true,
+    },


legend props are defined in DeclarativeChart.tsx. Use that only.

AtishayMsft · 2024-12-23T13:06:56Z

packages/charts/react-charting/src/components/DonutChart/DonutChart.base.tsx

  dataPointCalloutProps?: IChartDataPoint;
  callOutAccessibilityData?: IAccessibilityProps;
+  selectedLegends: string[];


selectedLegends

can you resolve all addressed comments

AtishayMsft · 2024-12-23T13:09:27Z

packages/charts/react-charting/src/components/DonutChart/DonutChartRTL.test.tsx

+
+    const getById = queryAllByAttribute.bind(null, 'id');
+    expect(getById(container, /Pie.*?first/i)[0]).toHaveStyle('opacity: 1.0');
+    expect(getById(container, /Pie.*?second/i)[0]).toHaveStyle('opacity: 1.0');


this is good. Thanks.

Legends multi selection for Donut Charts

b0c7663

srmukher requested a review from a team as a code owner December 11, 2024 11:37

fabricteam reviewed Dec 11, 2024

View reviewed changes

packages/charts/react-charting/src/components/DonutChart/Arc/Arc.tsx Show resolved Hide resolved

srmukher added 2 commits December 12, 2024 07:27

Making legend multi selection optional

24ce6bf

Showing total values when multiple legends are selected

434fc6d

AtishayMsft reviewed Dec 12, 2024

View reviewed changes

srmukher added 4 commits December 12, 2024 10:56

Separating hover and multi selection and adding example

70b0404

Merge branch 'master' into users/srmukher/LegendsMultiSel

addca43

Taking multiple legend selection from legend component callback

7293f33

Merge branch 'users/srmukher/LegendsMultiSel' of https://github.com/m…

ae348e2

…icrosoft/fluentui into users/srmukher/LegendsMultiSel

AtishayMsft reviewed Dec 16, 2024

View reviewed changes

srmukher added 4 commits December 17, 2024 08:04

Modifying handlers

88eb247

Merge branch 'master' of https://github.com/microsoft/fluentui into u…

db555d3

…sers/srmukher/LegendsMultiSel

Merge branch 'master' into users/srmukher/LegendsMultiSel

4fdc773

Merge branch 'master' into users/srmukher/LegendsMultiSel

a1dc7a9

AtishayMsft reviewed Dec 18, 2024

View reviewed changes

Removing canSelectMultipleLegends prop

13876f3

AtishayMsft reviewed Dec 18, 2024

View reviewed changes

Rearranging states and handlers

cea2d29

srmukher force-pushed the users/srmukher/LegendsMultiSel branch from d4339d8 to 6cbdf51 Compare December 19, 2024 13:20

Fixing donut labels

be186ba

srmukher force-pushed the users/srmukher/LegendsMultiSel branch from 6cbdf51 to be186ba Compare December 19, 2024 13:38

srmukher added 3 commits December 19, 2024 13:40

Add change file

d4641c5

Merge branch 'master' into users/srmukher/LegendsMultiSel

0e62c96

Fixing tests

568ca4e

srmukher force-pushed the users/srmukher/LegendsMultiSel branch from ca45bd3 to 943cff8 Compare December 20, 2024 09:55

AtishayMsft reviewed Dec 20, 2024

View reviewed changes

Merge branch 'users/srmukher/LegendsMultiSel' of https://github.com/m…

36e6bdc

…icrosoft/fluentui into users/srmukher/LegendsMultiSel

srmukher force-pushed the users/srmukher/LegendsMultiSel branch from 943cff8 to 36e6bdc Compare December 22, 2024 14:06

srmukher changed the title ~~Legends multi selection for Donut Charts~~ [Donut] Legends multi selection for Donut Charts Dec 23, 2024

srmukher added 2 commits December 23, 2024 12:07

Adding and updating tests

a94e9f1

Moving checks inside function

cfd09ea

srmukher force-pushed the users/srmukher/LegendsMultiSel branch from 6f40ac0 to cfd09ea Compare December 23, 2024 12:20

Merge branch 'master' into users/srmukher/LegendsMultiSel

0e7fc13

AtishayMsft reviewed Dec 23, 2024

View reviewed changes

[Donut] Legends multi selection for Donut Charts #33447

Are you sure you want to change the base?

[Donut] Legends multi selection for Donut Charts #33447

Conversation

srmukher commented Dec 11, 2024

github-actions bot commented Dec 11, 2024 • edited Loading

📊 Bundle size report

github-actions bot commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtishayMsft Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 11, 2024 •

edited

Loading

AtishayMsft Dec 20, 2024 •

edited

Loading