feat(view): add FileIcicleSummary #94

taejs · 2022-09-07T01:17:17Z

#37 [view] Statistics - FileIcicleSummary 구현

FileIcicleSummary는 커밋 클러스터별 변경된 파일을 icicle tree 형식으로 제공하는 컴포넌트입니다.

Description

chore(view): copy fake data from githru FileIcicleSummary.tsx

데이터 가공쪽은 다른 컴포넌트 작업되면 진행하는것이 효율적일거서 같아서 미리 작업하지는 않았고 임시 데이터를 fileChanges.json이라는 파일로 추가하여 사용했습니다.

fileChanges.json은 githuru - FileIcicleSummary.js >line 57의 fileAnalyzer.getFileStructure() 결과값을 복사한 값입니다. 코드 링크

chore(view): create FileIcicleSummary component

코드가 들어갈 파일을 생성했습니다.
파일 위치는 같은 Statistics 컴포넌트인 AuthorBarChart 위치와 동일하게 Statistics 하위에 만들었습니다.

feature(view): draw icicle tree in

데이터 기반으로 IcicleTree를 그립니다.
https://observablehq.com/@d3/zoomable-icicle 를 참고했습니다.
width, height는 각각 600, 400으로 임의로 정했습니다.

차트 그리는 부분은 githru-tutorial에서 진행한 내용과 크게 다르지 않아서 영어 주석이 부족하신 경우 githru/githru-boot#18 (comment) PR 내용도 함께 봐주시면 좋을것 같습니다.

feature(view): make to be zoomable

FileIcicleSummary의 인터랙션을 구현했습니다.
각 노드를 클릭했을때 해당 노드가 포커싱되고, 포커싱된 노드를 한번더 클릭하면 상위 노드로 포커싱됩니다.
이 부분 깔끔하게 개발하는 방법을 고민하다가 PR이 늦어졌네요

로직자체는 https://observablehq.com/@d3/zoomable-icicle 의 function clicked를 참고했습니다.

해당 예제에서는 각 node에 target이라는 필드를 추가해 변경된 위치정보를 저장하여 node를 mutable하게 사용하고 있습니다.

    root.each(d => d.target = {
      x0: (d.x0 - p.x0) / (p.x1 - p.x0) * height,
      x1: (d.x1 - p.x0) / (p.x1 - p.x0) * height,
      y0: d.y0 - p.y0,
      y1: d.y1 - p.y0
    });

각 node는 HierarchyRectangularNode 타입으로 추론되어 여기에 필드를 임의로 추가할경우 타입 오류가 발생하기도하고.. 해당 값은 인터랙션 발생시에만 사용하게 되기때문에 다른 방법을 찾아야했습니다.

positionMap이라는 WeakMap을 만들어 해당 노드를 key, 변경된 좌표값을 value로 설정하고 cell, rect, text, tspan쪽 값 변경할때는 positionMap의 value를 사용하도록 수정했습니다.

이렇게 변경하면 하위 노드들을 변경하지않고도 인터랙션을 구현할 수 있습니다.

feature(view): hide root node of partitions - FileIcicleSummary

githru에도 적용이 되어있는 내용으로 root node(레포지토리 최상위 디렉토리)는 노출하지 않도록 개발했습니다.

첫번째 변경사항인 transform쪽은 초기렌더링 x좌표를 설정하는 부분입니다.
root node가 보이지 않도록 SINGLE_RECT_WIDTH(각 노드 너비)만큼 제해줍니다.

두번째 변경사항은 인터랙션 발생 부분입니다. 위에도 설명했듯이 포커싱된 노드를 한번더 클릭하면 상위 노드로 포커싱되는 기능이 있습니다.

이부분때문에 depth가 2인 노드들을 한번더 클릭하면 root node로 포커싱이 되는데 이를 막기 위해서 isRootFocused가 true인 경우 x좌표를 SINGLE_RECT_WIDTH(각 노드 너비)만큼 제해서 root node가 보이지 않도록 개발했습니다.

실제 구현 화면

참고한 githru 화면 https://githru.github.io/vuejs-demo/
https://github.com/githru/githru/blob/b479984a3081e4e6e389179b86a6997db984d5a3/frontend/src/components/FileIcicleSummary.js#L8

Refers githru - https://github.com/githru/githru/blob/b479984a3081e4e6e389179b86a6997db984d5a3/frontend/src/components/FileIcicleSummary.js#L8

jeonghye-choi

고생하셨습니다~ 🤗

pshdev1030

와 코드 작성하신거 엄청 깔끔하네요..!
주석 작성해주신것도 너무 좋은 것 같아요.
고생하셨습니다..!!

ytaek

LGGGTM! interaction쪽 넘 좋네요!

ytaek · 2022-09-07T02:02:49Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+    return () => {
+      destroyIcicleTree($summary);
+    };


(궁금) unmount할때마다 cleanup되는거면, 여기서는 data가 바뀌게 될에 cleanup이 호출된다고 보면 될까요?

useEffect의 deps에 data를 넣어주어 생각하신대로 동작할것 같습니다!

ytaek · 2022-09-07T02:04:56Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+  file: "#ffe082",
+};
+
+type TFileData = {


type 네이밍 rule이 있었던 것 같은데, @githru/view 에서 한 번 확인부탁드립니당!

혹시 이 부분에 대해서 컨벤션 논의된 링크 아시는분 계실까요 ? 자료를 못찾겠어서 혹시 아시는 분 링크좀 공유해주시면 감사하겠습니다. wiki 컨벤션에 기록하게요. 🙇🏻‍♂️

ytaek · 2022-09-07T02:21:43Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+
+  const svg = d3
+    .select($target.current)
+    .attr("viewBox", [0, 0, WIDTH, HEIGHT])


viewBox 좋습니다 👍
font 크기가 좀 많이 줄어드는 부분이 있는데, 기본 길이나 반응형에 대한 걸 좀 고려해보면 좋겠네요.

ytaek · 2022-09-07T02:39:48Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+    // Initialize data - mutating before draw file icicle tree
+    // https://github.com/d3/d3-hierarchy/blob/v3.1.2/README.md#hierarchy
+    // https://observablehq.com/@d3/visiting-a-d3-hierarchy#count
+    .sum((d) => d?.value ?? 0)


사소한건데, d3node의 d와 data의 d가 약간 헷갈리는 것 같아요.
짧은게 좋아서 저도 d로 자주 사용하는데,

현재는 d3node에도 .value가 있고, node안의 data 에도 .value가 있으니 그게 헷갈리는 느낌?
이건 @githru/view 팀 전체가 같이 얘기를 해봐도 좋을 것 같습니다!

ytaek · 2022-09-07T02:41:25Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+const COLOR_CODE = {
+  dir: "#ffcc80",
+  file: "#ffe082",
+};


color scheme도 추후에 한번 통일해서 얘기해보면 좋겠습니당.

저희가 지난 회의때 상수들을 아예 파일 분리하자고 얘기했었습니다.
#55
discussion 확인부탁드릴게요!

컨벤션에 관한 문서는 계속 리스트업중에 있습니다. @__@
언능 정리해서 wiki에 올릴게연~ 🙇🏻‍♂️

jin-Pro · 2022-09-07T04:44:47Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+const COLOR_CODE = {
+  dir: "#ffcc80",
+  file: "#ffe082",
+};


저희가 지난 회의때 상수들을 아예 파일 분리하자고 얘기했었습니다.
#55
discussion 확인부탁드릴게요!

jin-Pro · 2022-09-07T04:48:06Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+    return () => {
+      destroyIcicleTree($summary);
+    };


useEffect의 deps에 data를 넣어주어 생각하신대로 동작할것 같습니다!

jin-Pro · 2022-09-07T04:49:37Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+import type { RefObject } from "react";
+import { useEffect, useRef, useState } from "react";
+
+const FILE_PATH = "/fileChanges.json";


우선 fake-asset으로 구현하고 컴포넌트에 맞게 각자 가공해서 구현하자고 얘기를 진행했었는데요,
따로 json 파일을 가지고오셔서 사용한 자세한 이유가 있을까요??

vgihan

feature practice 때 icicle chart를 구현하면서, 계산(x, y 좌표 및 width, height) 이후에 계산된 값을 attr 로 반영할 때, 코드가 많이 복잡해지고 읽기 힘들어진다라는 느낌을 제 코드 보고 많이 느꼈었습니다. 그래서 근본적인 원인을 생각해봤는데, 계산하는 로직에 대한 추상화가 제대로 이루어지지 않아서 그렇다는 느낌을 받았어요. 특히 클릭했을 때 rect가 확대되는 부분에서 제가 작성한 코드는 좌표 값을 재 계산한 이후에 attr에 넣어줄 때 한 번더 다른 계산이 들어가는데, 태림님 코드는 추상화가 잘 이루어져있어서 보는데 너무 깔끔하다는 생각이 들었습니다. 특히 WeakMap으로 변경되는 좌표를 모두 갖고 있다가 한 번에 반영해주는 부분과, draw함수로 분리해서 useEffect 내부를 간략하게 만드는 부분 너무 인상깊었습니다...! 인상깊은 부분을 리뷰에 하나하나 남기게 되면 코드에 제 리뷰만 가득찰 것 같아서 코멘트로 남깁니다...! 너무 잘읽었습니다 :)

hanseul-lee

수고하셨습니다~! LGTM!!👍👍👍

hanseul-lee · 2022-09-07T06:26:29Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+      insertion: number;
+      deletions: number;
+      count: number;


(궁금) deletions만 복수형인것은 의도하신 건가요?

hanseul-lee · 2022-09-07T06:37:35Z

packages/view/src/components/Statistics/FileIcicleSummary/FileIcicleSummary.tsx

+  useEffect(() => {
+    if (data) {
+      drawIcicleTree($summary, data);
+    }
+
+    // cleanup
+    return () => {
+      destroyIcicleTree($summary);
+    };


이부분 코드 너무 깔끔하고 좋은 거 같습니다👍👍👍
저도 비슷하게 리팩토링 해보겠습니다~

Suggested change

useEffect(() => {

if (data) {

drawIcicleTree($summary, data);

}

// cleanup

return () => {

destroyIcicleTree($summary);

};

useEffect(() => {

data && drawIcicleTree($summary, data);

// cleanup

return () => destroyIcicleTree($summary);

}, [data]);

요렇게 작성하는 것은 어떻게 생각하세용??

개인적으로 코드가 간결해지는 방향으로 작성하는 편이라서 여쭤봐용 !

hanseul-lee · 2022-09-07T09:25:24Z

@all-contributors please add @taejs for code

allcontributors · 2022-09-07T09:25:33Z

@hanseul-lee

I've put up a pull request to add @taejs! 🎉

jejecrunch

LGTM!!
인터랙션 너무 좋네요 !!
코드도 깔끔해서 많이 배우고 갑니다 !!

wherehows · 2022-09-07T11:32:56Z

크... 너무 멋있고 이쁘게 구현하셨네용... 고생하셨습니다 !!!! ( _ _ )

taejs added 5 commits September 6, 2022 23:14

chore(view): copy fake data from githru FileIcicleSummary.tsx

40c1a5f

chore(view): create FileIcicleSummary component

6174aab

feature(view): draw icicle tree in <FileIcicleSummary/>

0b4511d

feature(view): make <FileIcicleSummary/> to be zoomable

8f4d672

feature(view): hide root node of partitions - FileIcicleSummary

f06ee66

Refers githru - https://github.com/githru/githru/blob/b479984a3081e4e6e389179b86a6997db984d5a3/frontend/src/components/FileIcicleSummary.js#L8

taejs self-assigned this Sep 7, 2022

taejs requested a review from a team as a code owner September 7, 2022 01:17

jeonghye-choi approved these changes Sep 7, 2022

View reviewed changes

jeonghye-choi added the ✨ view label Sep 7, 2022

pshdev1030 approved these changes Sep 7, 2022

View reviewed changes

ytaek approved these changes Sep 7, 2022

View reviewed changes

jin-Pro reviewed Sep 7, 2022

View reviewed changes

vgihan approved these changes Sep 7, 2022

View reviewed changes

hanseul-lee approved these changes Sep 7, 2022

View reviewed changes

allcontributors bot mentioned this pull request Sep 7, 2022

docs: add taejs as a contributor for code #96

Merged

jejecrunch reviewed Sep 7, 2022

View reviewed changes

jejecrunch approved these changes Sep 7, 2022

View reviewed changes

ytaek added this to the v0.1.0 milestone Sep 11, 2022

jin-Pro merged commit ab7f798 into githru:main Sep 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(view): add FileIcicleSummary #94

feat(view): add FileIcicleSummary #94

taejs commented Sep 7, 2022

jeonghye-choi left a comment

pshdev1030 left a comment •

edited

Loading

ytaek left a comment •

edited

Loading

ytaek Sep 7, 2022

jin-Pro Sep 7, 2022

ytaek Sep 7, 2022

wherehows Sep 7, 2022 •

edited

Loading

ytaek Sep 7, 2022

ytaek Sep 7, 2022

ytaek Sep 7, 2022

jin-Pro Sep 7, 2022

wherehows Sep 7, 2022

jin-Pro Sep 7, 2022

jin-Pro Sep 7, 2022

jin-Pro Sep 7, 2022

vgihan left a comment

hanseul-lee left a comment

hanseul-lee Sep 7, 2022

hanseul-lee Sep 7, 2022

wherehows Sep 7, 2022 •

edited

Loading

hanseul-lee commented Sep 7, 2022

allcontributors bot commented Sep 7, 2022

jejecrunch left a comment

wherehows commented Sep 7, 2022

feat(view): add FileIcicleSummary #94

feat(view): add FileIcicleSummary #94

Conversation

taejs commented Sep 7, 2022

#37 [view] Statistics - FileIcicleSummary 구현

Description

chore(view): copy fake data from githru FileIcicleSummary.tsx

chore(view): create FileIcicleSummary component

feature(view): draw icicle tree in

feature(view): make to be zoomable

feature(view): hide root node of partitions - FileIcicleSummary

jeonghye-choi left a comment

Choose a reason for hiding this comment

pshdev1030 left a comment • edited Loading

Choose a reason for hiding this comment

ytaek left a comment • 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

wherehows Sep 7, 2022 • 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

vgihan left a comment

Choose a reason for hiding this comment

hanseul-lee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wherehows Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

hanseul-lee commented Sep 7, 2022

allcontributors bot commented Sep 7, 2022

jejecrunch left a comment

Choose a reason for hiding this comment

wherehows commented Sep 7, 2022

pshdev1030 left a comment •

edited

Loading

ytaek left a comment •

edited

Loading

wherehows Sep 7, 2022 •

edited

Loading

wherehows Sep 7, 2022 •

edited

Loading