Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat(plugin-chart-echarts): [feature-parity] support double clicking legend and series to view single selected series #1324

Merged
merged 7 commits into from
Sep 24, 2021

Conversation

stephenLYZ
Copy link
Contributor

@stephenLYZ stephenLYZ commented Aug 21, 2021

🏆 Enhancements
This PR supports two features:

  1. when user double click on the legend-> select single series, no change on ad hoc filter field -> click again to show all series.
  2. when user double click on a single series(in place filtering) -> display the correlative legend only -> no change on ad hoc filter field -> double click again to show all series

related to:

Line related chart (area, line, smooth, step)

Stacked

2021-09-05.11.49.51.mov

Unstacked

2021-09-05.11.49.05.mov

Bar chart

Stacked

2021-09-05.11.42.47.mov

Unstacked

2021-09-05.11.43.50.mov

Scatter Plot

2021-09-05.11.52.44.mov

@stephenLYZ stephenLYZ requested a review from a team as a code owner August 21, 2021 14:38
@vercel
Copy link

vercel bot commented Aug 21, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/BC2TaRZof4WheW6sS2wyUCppLdKj
✅ Preview: https://superset-ui-git-fork-stephenlyz-feat-support-dbclick-superset.vercel.app

legendselectchanged: payload => {
const currentTime = Date.now();
// 300 is the interval between two legendselectchanged event
if (currentTime - lastTimeRef.current < 300 && lastSelectedLegend.current === payload.name) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we can't pass click events (both click and double-click) to legend to drive the series change, we listen to the legendselectchanged and the time interval to simulate the double-click event. The value of 300 can be adjusted if user think is so fast.

}
});
} else {
lastTimeRef.current = currentTime;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we need to remember the legend and the time of the last click.

lastSelectedLegend.current = payload.name;
}
// if all legend is unselected, we keep all selected
if (Object.values(payload.selected).every(i => !i)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is keeping the same logic as NVD3.

const divRef = useRef<HTMLDivElement>(null);
const chartRef = useRef<ECharts>();
const currentSelection = Object.keys(selectedValues) || [];
const previousSelection = useRef<string[]>([]);

useImperativeHandle(ref, () => ({
Copy link
Contributor Author

@stephenLYZ stephenLYZ Aug 21, 2021

Choose a reason for hiding this comment

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

Here we pass echart instance to the parent component via ref

@codecov
Copy link

codecov bot commented Aug 21, 2021

Codecov Report

Merging #1324 (f119b18) into master (30a4c74) will decrease coverage by 0.17%.
The diff coverage is 1.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1324      +/-   ##
==========================================
- Coverage   30.49%   30.31%   -0.18%     
==========================================
  Files         496      496              
  Lines        9914     9974      +60     
  Branches     1666     1678      +12     
==========================================
+ Hits         3023     3024       +1     
- Misses       6647     6706      +59     
  Partials      244      244              
Impacted Files Coverage Δ
...chart-echarts/src/Timeseries/EchartsTimeseries.tsx 0.00% <0.00%> (ø)
...ins/plugin-chart-echarts/src/components/Echart.tsx 0.00% <0.00%> (ø)
plugins/plugin-chart-echarts/src/types.ts 100.00% <ø> (ø)
...gin-chart-echarts/src/Timeseries/transformProps.ts 53.94% <33.33%> (+0.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30a4c74...f119b18. Read the comment docs.

Copy link
Contributor

@pkdotson pkdotson left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally and works great! Thanks for the comments to help review the code! I would also suggest another charts team member to review this as well!

@stephenLYZ stephenLYZ changed the title feat(plugin-chart-echarts): support double clicking legend to single select series feat(plugin-chart-echarts): [feature-parity] support double clicking legend to single select series Aug 30, 2021
@pull-request-size pull-request-size bot added size/L and removed size/M labels Sep 5, 2021
@stephenLYZ stephenLYZ changed the title feat(plugin-chart-echarts): [feature-parity] support double clicking legend to single select series feat(plugin-chart-echarts): [feature-parity] support double clicking legend and series to view single selected series Sep 5, 2021
},
};

const zrEventHandlers: EventHandlers = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we use zrender event (https://echarts.apache.org/handbook/en/concepts/event/#listen-to-events-on-the-blank-area) instead of echart event, because we need to click on non-series areas to filter the series, such as stacked areas.

}
}, []);

const getModelInfo = (target: ViewRootGroup, globalModel: GlobalModel) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function involves the low-level processing logic of echarts and serves to obtain the series model.

@junlincc junlincc merged commit 90463d2 into apache-superset:master Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants