feat: rangeSelection feature on LineType, Column Chart #763

jwlee1108 · 2022-11-15T00:52:08Z

Please check if the PR fulfills these requirements

It's submitted to right branch according to our branching model
It's right issue type on title
When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
The commit message follows our guidelines
Tests for the changes have been added (for bug fixes/features)
Docs have been added/updated (for bug fixes/features)
It does not introduce a breaking change or has description for the breaking change

Description

add rangeSelection custom event interface

chart.on('rangeSelection', (selectedRange) => {
   console.log(selectedRange);
});

Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

- selection with line type series using zoom logic - prototyping with using options in state - convert to rangeSelection from zoom with Line Chart - add rangeSelectable on column chart - add stories

heenakwag

리뷰 완료합니다.

rangeSelection를 하다가 마우스가 차트를 벗어나면 rangeSelection이 도중에 멈추네요. 차트를 벗어나도 rangeSelection이 계속 되어야 하지 않을까요?

heenakwag · 2022-11-15T04:14:23Z

apps/chart/src/charts/columnChart.ts

@@ -60,7 +61,8 @@ import { ColumnChartProps, SelectSeriesInfo } from '@t/charts';
 *       @param {number|string} [props.options.chart.width] - Chart width. 'auto' or if not write, the width of the parent container is followed. 'auto' or if not created, the width of the parent container is followed.
 *       @param {number|string} [props.options.chart.height] - Chart height. 'auto' or if not write, the width of the parent container is followed. 'auto' or if not created, the height of the parent container is followed.
 *     @param {Object} [props.options.series]
- *       @param {boolean} [props.options.series.selectable=false] - Whether to make selectable series or not.
+ *       @param {boolean} [props.options.series.selectable=false]


설명이 사라졌네요

heenakwag · 2022-11-15T04:19:34Z

apps/chart/src/charts/columnLineChart.ts

@@ -30,6 +30,7 @@ import HoveredSeries from '@src/component/hoveredSeries';
 import DataLabels from '@src/component/dataLabels';
 import Tooltip from '@src/component/tooltip';
 import Background from '@src/component/background';
+import RangeSelection from '@src/component/rangeSelection';
 import NoDataText from '@src/component/noDataText';

 import * as basicBrush from '@src/brushes/basic';


여기는 rangeSelectable 설명이 없어도 되나요??

heenakwag · 2022-11-15T04:20:01Z

apps/chart/src/charts/lineScatterChart.ts

@@ -17,7 +17,7 @@ import Title from '@src/component/title';
 import ExportMenu from '@src/component/exportMenu';
 import SelectedSeries from '@src/component/selectedSeries';
 import HoveredSeries from '@src/component/hoveredSeries';
-import Zoom from '@src/component/zoom';
+import RangeSelection from '@src/component/rangeSelection';
 import Background from '@src/component/background';
 import NoDataText from '@src/component/noDataText';



여기도 rangeSelectable 설명이 없네요

이건 Zoom 기능 지원이 제대로 안되는 차트인데 들어가 있었네요. 이번 PR에선 빼고 zoom 처리를 다시 확인해봐야겠어요.

(추가)@ts-ignore 걸고 zoomable 사용하는 사람들이 있을 수 있어서 우선 이 상태로 둘게요.

adhrinae

차트 리뷰는 거의 처음 하네요. 제가 볼 수 있는 한에서는 LGTM입니다.

apps/chart/src/component/rangeSelection.ts

apps/chart/src/store/legend.ts

jwlee1108 · 2022-11-15T08:43:11Z

@dotaitch

우선 기존 차트의 구조나 동작 자체가 차트를 구성하는 각 컴포넌트에 responder라는 사용자 마우스 이벤트 수신기를 달아서 처리합니다.
zoom이나 rangeSelection은 series에 연결된 responder를 사용해야 하고 마우스가 series 영역을 넘어가는 선이 되었을 땐 이벤트를 취소 시키는게 맞긴 합니다.

만약 이 영역을 벗어날 경우 다른 컴포넌트의 responder과 동작이 겹쳐 의도치 않은 문제가 발생할 수도 있어요.
마우스가 plot을 벗어나면 드래그 처리가 취소되는 건 기존에 zoom에서도 마찬가지인데 rangeSelection은 동작 특성상 불편함이 더 두드러지네요.

지금 생각나는대로면..responder가 확인하는 영역을 조금 넓힐 수는 있을 것 같은데 앞서 말씀드린 다른 컴포넌트들과의 충돌 가능성과 계산 로직 수정 범위를 보고 일정 내 처리 가능한지 확인 후 진행해보겠습니다.

jajugoguma

리뷰 완료합니다! 차트는 처음은데 꽤 어렵네요.. ㅎㅎㅎ

jwlee1108 · 2022-11-16T01:51:37Z

@dotaitch

responder 처리 영역을 차트 전역으로 바꾸면 실제로 plot 영역을 넘어가도 동작을 하기에 계산 로직도 조금 수정해봤는데
legend나 다른 컴포넌트의 responder가 반응하는 문제가 발생하네요. 영역 넘어가는 건 known issue로 봐야겠습니다.

jwlee1108 added 4 commits November 15, 2022 09:30

feat: add rangeSelection option to LineType, Column Chart

59bb59d

- selection with line type series using zoom logic - prototyping with using options in state - convert to rangeSelection from zoom with Line Chart - add rangeSelectable on column chart - add stories

feat: add rangeSelection to columnLineChart

72d0075

feat: add rangeSelection on jsdoc

339d29b

fix: type error

5ec8020

jwlee1108 requested review from adhrinae, heenakwag and jajugoguma November 15, 2022 00:59

heenakwag suggested changes Nov 15, 2022

View reviewed changes

fix: apply review

0f3e626

adhrinae approved these changes Nov 15, 2022

View reviewed changes

apps/chart/src/component/rangeSelection.ts Show resolved Hide resolved

apps/chart/src/store/legend.ts Outdated Show resolved Hide resolved

fix: revert store/legend to wrong commit

4578437

jajugoguma approved these changes Nov 16, 2022

View reviewed changes

sunyoungBae approved these changes Nov 16, 2022

View reviewed changes

jwlee1108 requested a review from heenakwag November 16, 2022 01:51

heenakwag approved these changes Nov 16, 2022

View reviewed changes

jwlee1108 merged commit 7eb22a2 into main Nov 16, 2022

jwlee1108 deleted the feat/selection branch November 16, 2022 02:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: rangeSelection feature on LineType, Column Chart #763

feat: rangeSelection feature on LineType, Column Chart #763

jwlee1108 commented Nov 15, 2022

heenakwag left a comment

heenakwag Nov 15, 2022

heenakwag Nov 15, 2022

heenakwag Nov 15, 2022

jwlee1108 Nov 15, 2022 •

edited

Loading

adhrinae left a comment

jwlee1108 commented Nov 15, 2022 •

edited

Loading

jajugoguma left a comment

jwlee1108 commented Nov 16, 2022

feat: rangeSelection feature on LineType, Column Chart #763

feat: rangeSelection feature on LineType, Column Chart #763

Conversation

jwlee1108 commented Nov 15, 2022

Please check if the PR fulfills these requirements

Description

heenakwag left a comment

Choose a reason for hiding this comment

heenakwag Nov 15, 2022

Choose a reason for hiding this comment

heenakwag Nov 15, 2022

Choose a reason for hiding this comment

heenakwag Nov 15, 2022

Choose a reason for hiding this comment

jwlee1108 Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

adhrinae left a comment

Choose a reason for hiding this comment

jwlee1108 commented Nov 15, 2022 • edited Loading

jajugoguma left a comment

Choose a reason for hiding this comment

jwlee1108 commented Nov 16, 2022

jwlee1108 Nov 15, 2022 •

edited

Loading

jwlee1108 commented Nov 15, 2022 •

edited

Loading