feat: painting bar/column color based on categories #705

jwlee1108 · 2021-09-14T02:59:35Z

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 feature that paints to bar color based on categories.

const data = {
  categories: ['Jun', 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec'],
  series: [
    {
      name: 'Budget',
      data: [5000, 3000, 5000, 7000, 6000, 4000, 1000],
      colorByCategories: true
    }
  ]
}

Add description about the option in docs
related issue: Need Single bar column chart #599

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

adhrinae

리뷰 완료합니다.

adhrinae · 2021-09-14T05:48:33Z

apps/chart/src/charts/barChart.ts

@@ -48,6 +48,7 @@ import { BarChartProps, SelectSeriesInfo } from '@t/charts';
 *     @param {Array<string>} props.data.categories - Categories.
 *     @param {Array<Object>} props.data.series - Series data.
 *       @param {string} props.data.series.name - Series name.
+ *       @param {string} props.data.series.colorByCategories - Paint to Rect with color based on categories.


Paint to 에서 to 를 빼주세요.
같은 내용으로 되어있는 주석 모두 마찬가지입니다.

네 알겠습니다. 감사합니다!

adhrinae · 2021-09-14T06:05:52Z

apps/chart/src/store/legend.ts

-    ...datum,
-    color: colors[idx],
-  }));
+  const hasColorByCategories = some(data, ['colorByCategories', true]);


이건 data 중에 colorByCategories 가 true 인게 하나라도 있는지를 검사하는 로직인가요?

lodash말고 array.prototype.some 사용해도 되지 않나요?

차트에서 lodash를 사용하나요~? 이건 한번 확인 부탁드려요. 저도 배열 내장 메서드를 사용하는게 더 가독성에 좋을 것 같습니다.

내장 메서드로 변경할께요!

@adhrinae

네. 이건 약간 전체적인 입력값에 대응하기 위한 처리인데..예를 들어 컬럼-라인 콤보차트라고 하고 데이터가 다음과 같이 들어온다고 했을 때

{ category: ['Mon', 'Tue', 'Wed', 'Thu', 'Fri'], series: [{ type: 'bar', data: [100, 200, 300, 400, 500], colorByCategories: true }, { type: 'line', data: [500, 400, 300, 200, 100], }] }

colorByCategories가 없는 컬럼-라인 차트라면 color값이 시리즈 갯수만큼 2개만 필요할거예요.

colorByCategories가 true면 색이 총 6개가 필요합니다. (막대기 5개, 라인 1개)

colorByCateogories 시리즈는 특정 컬러로 단언할 수가 없기에 회색처리를 하고, 라인 혹은 옵션이 없는 시리즈의 경우엔 카테고리 갯수만큼 인덱스를 건너뛰어 컬러값을 찾아야 합니다.

그래서 여기 로직이 약간 복잡해졌어요.

adhrinae · 2021-09-14T06:07:06Z

apps/chart/src/store/legend.ts

      const label = colorValue ? colorValue : name;
+      const currentColorIndex = colorIndex;


이정도면 위에 선언된 변수가 그냥 currentColorIndex 여도 되지 않을까요?

중간에 값이 변경되니까 따로 할당한 것 같습니다ㅎㅎ

네 이게 중간에 값이 변경되어야 해서 애매했어요. 레전드 입장에선 컬러 리스트만 알고 있는 상황이고, 시리즈 단위로 컬러를 N개 가져가는 구조가 되다보니 다음 컬러 인덱스를 잡아줄 변수가 필요했습니다.

adhrinae · 2021-09-14T06:10:32Z

docs/en/chart-bar.md

@@ -187,6 +187,25 @@ If you create a chart by applying the above option, you can see that the checkbo

 ![image](https://user-images.githubusercontent.com/35371660/108007745-0ccf5980-7042-11eb-8eb8-ae7ed497c939.png)

+## colorByCategories 옵션


한글 발견

헛 수정하겠습니다.! 감사합니다.

adhrinae · 2021-09-14T06:11:47Z

docs/en/chart-bar.md

@@ -187,6 +187,25 @@ If you create a chart by applying the above option, you can see that the checkbo

 ![image](https://user-images.githubusercontent.com/35371660/108007745-0ccf5980-7042-11eb-8eb8-ae7ed497c939.png)

+## colorByCategories 옵션
+
+Each `series` can have `colorByCategories` option. The `colorByCategories` option determines whether to paint the bar color of the chart differently based on the categories. The default is `false`.


마지막에 The default 보다는 기본값이라는 의미에서 The default value 면 좀 더 명확하겠네요.

네 이 부분 변경하겠습니다.

adhrinae · 2021-09-14T06:12:47Z

docs/en/chart-columnLine.md

@@ -101,6 +101,33 @@ If you create a chart by applying the above option, you can see that the checkbo
 ![image](https://user-images.githubusercontent.com/35371660/108009092-3fc71c80-7045-11eb-901e-03d20fdee3dc.png)


+## colorByCategories 옵션


여기도 한글 발견

ㅠㅠㅠ 감사합니다.

lja1018

리뷰완료합니다.

lja1018 · 2021-09-14T07:25:16Z

apps/chart/src/store/legend.ts

+    const labelInfo = series[type].map((m) => {
+      const { name, colorValue, visible, colorByCategories } = m;


Suggested change

const labelInfo = series[type].map((m) => {

const { name, colorValue, visible, colorByCategories } = m;

const labelInfo = series[type].map(({ name, colorValue, visible, colorByCategories }) => {

로 해도 될것 같습니다.

@lja1018 @js87zz 이 방향으로 수정할께요

lja1018 · 2021-09-14T07:28:38Z

apps/chart/src/store/legend.ts

-    ...datum,
-    color: colors[idx],
-  }));
+  const hasColorByCategories = some(data, ['colorByCategories', true]);


lodash말고 array.prototype.some 사용해도 되지 않나요?

lja1018 · 2021-09-14T07:29:21Z

apps/chart/src/store/legend.ts

+    const { colorByCategories, colorIndex } = datum;
+    const index = hasColorByCategories ? colorIndex || idx : idx;


Suggested change

const { colorByCategories, colorIndex } = datum;

const index = hasColorByCategories ? colorIndex || idx : idx;

const { colorByCategories, colorIndex = idx } = datum;

const index = hasColorByCategories ? colorIndex : idx;

는 안될까요?

colorIndex || idx이면 0인 경우에도 변경되는 건데 의도된 동작인거죠~?

0인 경우에도 문제가 없으리라 판단했는데 이 부분은 조금 더 확인해보겠습니다.

@lja1018 colorByCateogories가 없을 땐 기존대로 가야해서 조금 어려울 것 같아요. 확인 후 가능하면 코드를 좀 더 정리해보겠습니다.

heenakwag

리뷰 완료합니다

jajugoguma

리뷰 완료 합니다.

js87zz

늦었지만..ㅠ리뷰 완료입니다!

js87zz · 2021-09-15T07:57:08Z

apps/chart/src/component/boxStackSeries.ts

@@ -378,7 +380,7 @@ export default class BoxStackSeries extends BoxSeries {
  ): TooltipData[] {
    const seriesRawData = seriesData.data;
    const { stackData } = seriesData;
-    const colors = seriesRawData.map(({ color }) => color);
+    const colors = seriesRawData.map(({ color }) => color) as string[];


기존에도 color가 string 타입이란 가정하에 동작했던 건가요? 혹시 몰라서 여쭤봅니다.

네. 각 시리즈에 color값이 묶이는 구조인데 이번엔 컬러값이 배열로 들어와야 해서, 변수를 추가할까 타입을 추가할까 하다가 타입을 추가했어요.

js87zz · 2021-09-15T08:00:01Z

apps/chart/src/store/legend.ts

-  return Object.keys(series).flatMap((type) =>
-    series[type].map(({ name, colorValue, visible }) => {
+  return Object.keys(series).flatMap((type) => {
+    const labelInfo = series[type].map((m) => {


m이 any 타입으로 추론되는데 한 번 확인 부탁드려요. 그리고 파라미터 명이 어떤 건지 잘 와닿지 않아서 명확한 네이밍으로 변경되면 좋겠습니다~!

js87zz · 2021-09-15T08:01:35Z

apps/chart/src/store/legend.ts

      const label = colorValue ? colorValue : name;
+      const currentColorIndex = colorIndex;


중간에 값이 변경되니까 따로 할당한 것 같습니다ㅎㅎ

js87zz · 2021-09-15T08:03:08Z

apps/chart/src/store/legend.ts

-    ...datum,
-    color: colors[idx],
-  }));
+  const hasColorByCategories = some(data, ['colorByCategories', true]);


차트에서 lodash를 사용하나요~? 이건 한번 확인 부탁드려요. 저도 배열 내장 메서드를 사용하는게 더 가독성에 좋을 것 같습니다.

js87zz · 2021-09-15T08:05:55Z

apps/chart/src/store/legend.ts

+    const { colorByCategories, colorIndex } = datum;
+    const index = hasColorByCategories ? colorIndex || idx : idx;


colorIndex || idx이면 0인 경우에도 변경되는 건데 의도된 동작인거죠~?

js87zz · 2021-09-15T08:07:23Z

apps/chart/src/store/seriesData.ts

-          color: colors ? colors[idx % colors.length] : '',
-        }));
+
+        let originSeriesData = rawSeries[seriesName].map((m) => {


이 부분도 m보다는 더 명확한 네이밍으로 변경되면 좋을 것 같습니다.

네 변경하겠습니다~!

js87zz · 2021-09-15T08:10:15Z

apps/chart/src/store/seriesData.ts

+            ? getSeriesColors(colors, colorIndex, size, isColorByCategories)
+            : '';
+
+          colorIndex += size;


이 연산 이후로 colorIndex를 쓰는 곳이 없어보이는데 연산하는 이유가 있나요~?

loop로 돌면서 195라인에서 계속 데이터를 사용하고 있어요. 이 함수 밖에서 사용 중입니다. reduce로 할까 했는데, 제 생각엔 map 성격이 더 강해보여서 함수 외부에 변수를 두고 누적 계산하는 식으로 처리했습니다.

js87zz · 2021-09-15T08:14:22Z

apps/chart/src/store/theme.ts

@@ -1,4 +1,5 @@
-import { Options, RawSeries, StoreModule } from '@t/store/store';
+import { filter, reject } from 'lodash';


이 부분도 lodash 사용 유무 확인 부탁드려요. 제가 알기로는 차트 쪽 util 파일에서 이런 코드 다 직접 만들어서 쓰는 걸로 알고 있어서요~ package.json에도 없는 거 보니 다른 의존성 때문에 설치되어서 쓸 수 있는 것 같아요

네 이 부분은 변경할께요

js87zz · 2021-09-15T08:21:50Z

docs/ko/chart-bar.md

+
+## colorByCategories 옵션
+
+각각의 `series`는 `colorByCategories` 옵션을 가질 수 있다. `colorByCategories` 옵션은 차트의 막대 색을 카테고리별로 다르게 칠하는 여부를 결정한다. 기본값은 `false`이다.


막대 색을 카테고리별로 다르게 칠하는 여부를 결정한다. -> 막대 색을 카테고리 별로 다르게 칠할지 결정한다.

는 어떤가요?

더 명확해보이네요 반영하겠습니다. 감사합니다.

jwlee1108 and others added 6 commits September 12, 2021 14:17

feat: colorByCategories - basic feature

73496d9

stack chart

943d8ed

feat: colorByCategories - combo

38de582

feat: colorByCategories - docs

b0830c1

feat: colorByCategories - docs en

1a8518d

test code

24567a8

adhrinae reviewed Sep 14, 2021

View reviewed changes

lja1018 reviewed Sep 14, 2021

View reviewed changes

heenakwag approved these changes Sep 15, 2021

View reviewed changes

jajugoguma approved these changes Sep 15, 2021

View reviewed changes

js87zz approved these changes Sep 15, 2021

View reviewed changes

jwlee1108 added 2 commits September 17, 2021 02:17

apply review

a013a10

fix test error

8a2ddc7

jwlee1108 merged commit 81e9691 into main Sep 17, 2021

jwlee1108 deleted the feat/color-by-categories branch September 17, 2021 09:16

		const label = colorValue ? colorValue : name;
		const currentColorIndex = colorIndex;

		@@ -187,6 +187,25 @@ If you create a chart by applying the above option, you can see that the checkbo

		![image](https://user-images.githubusercontent.com/35371660/108007745-0ccf5980-7042-11eb-8eb8-ae7ed497c939.png)

		## colorByCategories 옵션

		@@ -101,6 +101,33 @@ If you create a chart by applying the above option, you can see that the checkbo
		![image](https://user-images.githubusercontent.com/35371660/108009092-3fc71c80-7045-11eb-901e-03d20fdee3dc.png)


		## colorByCategories 옵션

		const labelInfo = series[type].map((m) => {
		const { name, colorValue, visible, colorByCategories } = m;

	const labelInfo = series[type].map((m) => {
	const { name, colorValue, visible, colorByCategories } = m;
	const labelInfo = series[type].map(({ name, colorValue, visible, colorByCategories }) => {

		const { colorByCategories, colorIndex } = datum;
		const index = hasColorByCategories ? colorIndex \|\| idx : idx;

		@@ -1,4 +1,5 @@
		import { Options, RawSeries, StoreModule } from '@t/store/store';
		import { filter, reject } from 'lodash';


		## colorByCategories 옵션

		각각의 `series`는 `colorByCategories` 옵션을 가질 수 있다. `colorByCategories` 옵션은 차트의 막대 색을 카테고리별로 다르게 칠하는 여부를 결정한다. 기본값은 `false`이다.

feat: painting bar/column color based on categories #705

feat: painting bar/column color based on categories #705

Conversation

jwlee1108 commented Sep 14, 2021 • edited Loading

Please check if the PR fulfills these requirements

Description

adhrinae left a 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

lja1018 left a 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

heenakwag left a comment

Choose a reason for hiding this comment

jajugoguma left a comment

Choose a reason for hiding this comment

js87zz left a 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

jwlee1108 commented Sep 14, 2021 •

edited

Loading