Fix legends selection bugs by krkshitij · Pull Request #24563 · microsoft/fluentui

krkshitij · 2022-08-29T08:01:59Z

Current Behavior

Legend selection gets lost after hovering out of the overflow callout
Legend states in chart components don't stay in sync with the Legends component. For example, in case of multiple legend selection, wrong element gets highlighted when mouse hasn't left the last deselected legend
- Three states (selectedLegend, isLegendSelected, isLegendHovered) keep track of selection and hovering on legends together

New Behavior

Legends selection remains even after hovering out of the overflow callout
Legend states in chart components and the Legends component stay in sync
- Two states (selectedLegend, activeLegend) keep track of selection and hovering on legends separately

fabricteam · 2022-08-29T08:10:32Z

📊 Bundle size report

_{🤖 This report was generated against 4d58b78e55691de6f14746dbd282c315771be933}

codesandbox-ci · 2022-08-29T08:12:12Z

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 1b75d31:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

size-auditor · 2022-08-29T08:13:24Z

Asset size changes

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

Baseline commit: 4d58b78e55691de6f14746dbd282c315771be933 (build)

AtishayMsft · 2022-10-04T10:52:41Z

+    });
  }

-  private _onLegendLeave(isLegendFocused?: boolean): void {


Do you need to keep track of isLegendFocused flag.

No, it is not needed in the new logic

AtishayMsft · 2022-10-04T10:53:50Z

    this.setState({ isCircleClicked: true });
  };

-  private _getOpacity = (selectedArea: string): number => {


Is _getOpacity applicable for legends

No, it is only applicable for elements inside the chart

AtishayMsft · 2022-10-04T17:11:19Z

-        },
-      );
+      this.setState({ isHoverCardVisible: false });
+      const activeOverflowItem = find(legends, (legend: ILegend) => legend.title === this.state.activeLegend);


Is canSelectMultipleLegends not needed in the new logic

It is needed here:

But it is not required in the onHoverCardHideHandler function

AtishayMsft · 2022-10-04T17:14:55Z

-    ) {
-      legendColor = color;
+    const inSelectedState = this.state.selectedLegend !== '' || this.state.selectedLegends.length > 0;
+    if (inSelectedState) {


Could you share what all you have tested

The color of the legend is decided based on these 3 cases:

When 1 or more legends are selected: The selected legends have their respective colors but the unselected legends have white color

When no legend is selected but there is a hovered legend (the legend which has the mouse pointer over it): All legends except the hovered legend have white color

When no legend is selected or hovered (default case): All legends have their respective colors

fabricteam · 2022-10-26T16:17:30Z

Perf Analysis (`@fluentui/react-components`)

No significant results to display.

All results

Scenario	Render type	Master Ticks	PR Ticks	Iterations
Avatar	mount	1251	1311	5000
Button	mount	866	925	5000
FluentProvider	mount	1471	1500	5000
FluentProviderWithTheme	mount	625	568	10
FluentProviderWithTheme	virtual-rerender	592	561	10
FluentProviderWithTheme	virtual-rerender-with-unmount	550	630	10
MakeStyles	mount	1793	1923	50000
SpinButton	mount	2524	2515	5000

AtishayMsft · 2022-10-31T12:16:15Z

-      shouldHighlight = this.state.titleForHoverCard === legendTitle;
-    }
-    return shouldHighlight ? '' : '0.1';
+    const opacity = this._legendHighlighted(legendTitle) || this._noLegendHighlighted() ? '' : '0.1';


Can you confirm that this value is '' and not something else.

The browser treats empty opacity as 100%. Instead of an empty string, '1' would be more intuitive.

AtishayMsft · 2022-10-31T12:26:18Z

-      if (this.state.isLegendHovered || this.state.isLegendSelected) {
-        shouldHighlight = this.state.selectedLegendTitle === point.legend;
-      }
+      const shouldHighlight = this._legendHighlighted(point.legend!) || this._noLegendHighlighted() ? true : false;


Add brackets to clarify the scope of expression

I tried to add the brackets but they get removed automatically on formatting

* fix legends selection reset bug * sync LineChart legend states * sync AreaChart legend states * sync HeatMapChart legend states * sync StackedBarChart legend states * sync VerticalBarChart legend states * sync DonutChart legend states * sync GroupedVerticalBarChart legend states * sync VerticalStackedBarChart legend states * refactor * add change file * rename function * minor * add comments

msft-fluent-ui-bot added the Package: charting label Aug 29, 2022

msft-fluent-ui-bot assigned micahgodbolt Aug 29, 2022

krkshitij force-pushed the bug-legend-selection-reset branch from 7016020 to b56a750 Compare September 15, 2022 07:23

AtishayMsft reviewed Oct 4, 2022

View reviewed changes

Comment thread packages/react-charting/src/components/AreaChart/AreaChart.base.tsx

krkshitij marked this pull request as ready for review October 26, 2022 16:07

krkshitij requested review from a team, EisenbergEffect, chrisdholt, nicholasrice and sopranopillow as code owners October 26, 2022 16:07

krkshitij added 11 commits October 26, 2022 21:48

fix legends selection reset bug

f8a9ff0

sync LineChart legend states

09ecaca

sync AreaChart legend states

17a566b

sync HeatMapChart legend states

704d230

sync StackedBarChart legend states

802802d

sync VerticalBarChart legend states

25c9686

sync DonutChart legend states

208c8c1

sync GroupedVerticalBarChart legend states

1a88853

sync VerticalStackedBarChart legend states

c7ec0d3

refactor

3625b77

add change file

605f547

krkshitij added 2 commits October 26, 2022 21:59

rename function

9807339

minor

376a49e

krkshitij force-pushed the bug-legend-selection-reset branch from ab33291 to 376a49e Compare October 26, 2022 18:05

krkshitij removed request for a team, EisenbergEffect, chrisdholt, nicholasrice and sopranopillow October 26, 2022 18:07

add comments

1b75d31

AtishayMsft reviewed Oct 31, 2022

View reviewed changes

AtishayMsft approved these changes Nov 3, 2022

View reviewed changes

krkshitij merged commit 6fe1046 into microsoft:master Nov 3, 2022

AtishayMsft mentioned this pull request Dec 12, 2022

[Bug]: [Charting][Line chart]Filter is reset when mouse hovers out of 'more' dropdown list #24534

Closed

2 tasks

krkshitij mentioned this pull request Dec 15, 2022

Cherry-pick fix for wrong narration when legend selected #25936

Merged

krkshitij deleted the bug-legend-selection-reset branch February 8, 2023 10:42

Uh oh!

Conversation

krkshitij commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Behavior

New Behavior

Uh oh!

fabricteam commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Bundle size report

Uh oh!

codesandbox-ci Bot commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

size-auditor Bot commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Asset size changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krkshitij Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krkshitij Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabricteam commented Oct 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

krkshitij commented Aug 29, 2022 •

edited

Loading

fabricteam commented Aug 29, 2022 •

edited

Loading

codesandbox-ci Bot commented Aug 29, 2022 •

edited

Loading

size-auditor Bot commented Aug 29, 2022 •

edited

Loading

krkshitij Oct 24, 2022 •

edited

Loading

krkshitij Oct 24, 2022 •

edited

Loading