feat(view): fix verticalClusterList issues and refactor detail component #154

wherehows · 2022-09-12T14:56:54Z

Related Issue

[FIX REQUEST] Graph + Detail #146

📌 Key Changes

1. 시간을 보여주기 위한 함수 및 폴더 추가(08d5bd7)

현재 new Date 인자로 utc를 전달하여 로컬 타임으로 변환하는 방식을 사용하고 있습니다. new Date 인자로 전달하여 시간을 변환하는 것은 지양(아래 자료 참고)되어야 한다고 아는데, moment 나 dayjs 라이브러리 검토할 시간이 없어서 new Date 이용했습니다. 살짝 봤을때는 moment 는 더이상 유지보수가 안되고 불변성을 지키지 않고, 번들링 했을 때의 사이즈가 크기 때문에 dayjs 선택하는 것 같네요.
아래 링크에서는 다음과 같이 설명합니다.

생성자를 이용하는 또 하나의 방법은 문자열 데이터를 이용하는 것이다. Date 객체의 생성자에 문자열 타입의 값을 사용하면, 내부적으로 Date.parse()를 호출하여 적절한 값을 계산해내며, 이 함수는 RFC2888 스펙과 ISO-8601 스펙을 지원한다. 하지만 MDN의 Date.parse() 문서에도 나와있듯이, 이 메소드의 결과값은 브라우저마다 구현상태가 다르고, 문자열의 형태에 따라 정확한 값을 예측하기 어렵기 때문에 사용하지 않기를 권장하고 있다.
FYI: https://meetup.toast.com/posts/130

다른 분들도 사용하실 것 같아서 일단은 전역 utils로 빼두었고, 시간 포맷은 git lens의 commit graph를 참고했습니다.

2. detail 컴포넌트 추가적인 정보 추가(919a030)

detail 컴포넌트에 추가적인 정보, commit Id, author, date 를 추가했습니다.
이름이나 시간의 위치는 git lens를 참고했고, commit id는 github와 동일하게 배치했습니다.

3. collapsible 을 판단할 수 있는 마커 추가 (bde9696)

아이콘이 없어서 울며 겨자먹기로 ㅁ 특수문자 중 하나를 사용했습니다. 0.2.0부터는 아이콘이 추가되었으면 좋겠습니다.

4. authors, commits, changed files, changed line of codes에 대한 정보 간략화 (4ad24a4)

5. 가시성 향상 (5907153)

가시성 향상을 위해서 호버한 요소에 대해서 화살표와 백그라운드 컬러가 바뀌도록 했습니다.

6. Detail 컴포넌트 고정된 높이에서 가변 높이로 변경 (7271096)

📌 To Reviwers

버전 0.2.0 올라갈때는 아이콘 라이브러리 하나 가져오는게 어떨까요?
Detail 컴포넌트의 높이 변화를 ResizeObserver, selectedData의 변화로 캐치하고 있습니다. 옵저버를 등록했다가 제거햇다가 하는 로직이 깔끔하지 않습니다. 하지만 1.시간상, 2. 디테일 컴포넌트에서 더보기 버튼을 눌렀을 때 이벤트의 발생을 위에 알리려면 결국 prop driling이 발생해서 그대로 구현했습니다. 추후에 수정되어야 할것 같습니다.
commit 메세지가 길어지는 경우, 줄바꿈이 일어납니다. ellipsis하게 처리하고 싶지만 css 지식이 부족하여 처리하지 못했습니다.
(@jejecrunch , @vgihan) 충돌 해소할 때 잘 한다고 하긴 했는데, 불안해서요. 죄송하지만 제 능력이 부족해서 ㅠㅠ,, 전체적으로 조금 더 디테일하게 봐주시면 감사하겠습니다.

hanseul-lee · 2022-09-12T15:03:53Z

packages/view/src/components/Detail/Detail.tsx

+      <div className="divider" />
+      <div className="text">
+        <strong>{authorLength}</strong> authors |{" "}
+        <strong>{commitLength}</strong> commits | <strong>{fileLength}</strong>{" "}
+        changed files | <strong className="insertions">{insertions}</strong>{" "}
+        additions and <strong className="deletions">{deletions}</strong>{" "}
+        deletions codes
+      </div>


오 완전 깔끔해져서 보기 좋아요~👍

와 리뷰 엄청 빠르시네요...

hanseul-lee · 2022-09-12T15:05:34Z

packages/view/src/components/Detail/Detail.scss

+  }
+}
+
+.detail-commit_item_container {


혹시 이부분 비롯해 전반적인 class 네이밍 .detial-commit__item-container와 같이 바꿔주실 수 있으신가요?
제가 수정했는데 변경 사항이 많다보니 영후님이 먼저 반영해주시는 게 좋을 것 같습니다~!

BEM 룰 때문일까요 @__@?

hanseul-lee · 2022-09-12T15:07:25Z

packages/view/src/components/VerticalClusterList/VerticalClusterList.hook.ts

+export const useResizeObserver = (
+  ref: RefObject<HTMLDivElement>,
+  selectedData: SelectedDataProps
+) => {
+  const [height, setHeight] = useState(0);
+
+  useEffect(() => {
+    let RO: ResizeObserver | null = new ResizeObserver((entries) => {
+      if (!Array.isArray(entries)) {
+        return;
+      }
+      const { contentRect } = entries[0];
+      setHeight(contentRect.height);
+    });
+
+    if (ref.current) {
+      RO.observe(ref.current);
+    }
+
+    return () => {
+      if (RO) {
+        RO.disconnect();
+        RO = null;
+      }
+    };
+  }, [ref.current, selectedData]);
+
+  return [height];
+};


👍👍👍

와 이 부분 하셨군요 ! 감사합니다 ㅎㅎ

hanseul-lee · 2022-09-12T15:17:20Z

와 사진 추가해주신 거 뒤늦게 보았는데 2,4,5 부분 정말 좋네요~~👍👍👍

jejecrunch

제 부분은 제대로 합쳐진 것 같습니다 ! 고생하셨습니다 !! LGTM ㅎㅎ

jejecrunch · 2022-09-12T15:25:48Z

packages/view/src/components/VerticalClusterList/VerticalClusterList.hook.ts

+export const useResizeObserver = (
+  ref: RefObject<HTMLDivElement>,
+  selectedData: SelectedDataProps
+) => {
+  const [height, setHeight] = useState(0);
+
+  useEffect(() => {
+    let RO: ResizeObserver | null = new ResizeObserver((entries) => {
+      if (!Array.isArray(entries)) {
+        return;
+      }
+      const { contentRect } = entries[0];
+      setHeight(contentRect.height);
+    });
+
+    if (ref.current) {
+      RO.observe(ref.current);
+    }
+
+    return () => {
+      if (RO) {
+        RO.disconnect();
+        RO = null;
+      }
+    };
+  }, [ref.current, selectedData]);
+
+  return [height];
+};


와 이 부분 하셨군요 ! 감사합니다 ㅎㅎ

jejecrunch · 2022-09-12T15:27:34Z

packages/view/src/components/VerticalClusterList/Summary/Summary.scss

+    color: $white;
+  }
+
+  .summary:hover {


혹시 이 부분 summary 안으로 &:hover 이렇게 넣어주실 수 있으실까요?

@jejecrunch 저거 제가 안에 넣어서 하려고 했는데, SUMMARY 의 자식 요소들 각각만 파란색으로 배경색이 보이더라고요. summary 리스트 아이템 각각이 파란색 배경으로 보이는게 아니라요!

확인했습니다 ! 감사합니다 ㅎㅎ

jejecrunch · 2022-09-12T15:30:18Z

CI 오류 해결하시고 기한님도 확인해주시면 바로 합쳐도 괜찮을 것 같습니다 ! detail hook에서 에러 나는 것 같습니다 ㅜㅜ

jejecrunch · 2022-09-12T15:34:19Z

버전 0.2.0 올라갈때는 아이콘 라이브러리 하나 가져오는게 어떨까요?

이 부분 이슈로 등록해서 아이콘 라이브러리 2~3개 결정해서 투표해보는 건 어떨까요?

요 부분에서 커밋 아이디가 줄여보는 것도 괜찮을 것 같은데 어떠신가요? git graph에서는 커밋리스트를 보여줄 때는 총 8자리로 알려주고 있어서요 !

vgihan

AS-IS	TO-BE

ClusterGraph에 가변 길이를 적용함에 따라 발생하는 transition 문제가 존재합니다. Detail 컴포넌트를 summary_detail_container(이하 container) 로만 감싸게 되면, container는 transition이 발생하는 element이기 때문에 height 가 transition이 시작될 때 height가 가변적으로 변경됩니다. 이 때, resizeObserver는 애니메이션이 발생하는 element를 height가 바뀔 때마다 감지하게 되고, 그 결과로 위의 AS-IS와 같은 Graph의 떨림이 발생합니다. 그렇기 때문에 container와 Detail 컴포넌트 사이에 height 값이 변경되지 않는(애니메이션이 없는) div 태그를 추가하여 resizeObserver가 단 한 번만 height의 변경을 감지할 수 있도록 수정해야할 것 같습니다. 영후님 PR 머지해주시면 제가 후속 작업 이어서 진행해보도록 하겠습니다 ! 리뷰 부분은 지금 반영하지 않으셔도 됩니다 :)

vgihan · 2022-09-12T16:01:50Z

packages/view/src/components/VerticalClusterList/Summary/Summary.tsx

+                </button>
+                {cluster.clusterId === clusterIds && (
+                  <div className="summary_detail_container" ref={ref}>
+                    <Detail selectedData={selectedData} />


여기서 div 태그와 Detail 태그 사이에 div 태그를 하나 추가하여 transition이 발생하지 않는 태그에 ref를 걸어야 transition이 그나마 나아질 것 같습니다 ..! (자세한 내용은 코멘트를 참조해주세요)

또한, 사이에 추가되는 div 태그에 max-height: 280px을 걸어줘야 280px 이상으로 graph가 늘어나는 상황이 발생하지 않겠습니다.

제가 구현에서 놓친 부분인 것 같습니다. 리뷰 감사드립니당. ㅠㅠ

vgihan · 2022-09-12T16:02:34Z

packages/view/src/components/Detail/Detail.scss

+  display: flex;
+  justify-content: space-between;
+  align-items: center;
+  margin-top: 14px;


이 부분 margin-top에서 padding-top으로 바꿔야 아래에서 언급한 div 태그와 Detail 태그 사이에 들어가는 div 태그가 밀리지 않습니다 !

추후 반영하도록 하겠습니다. 감사합니당!

hanseul-lee · 2022-09-12T17:42:56Z

commit 메세지가 길어지는 경우, 줄바꿈이 일어납니다. ellipsis하게 처리하고 싶지만 css 지식이 부족하여 처리하지 못했습니다.

이 부분 머지해주시면 제가 수정해 올리겠습니다~!

ytaek

와우!! 엄청 디테일이 좋아지는 것 같습니다.

덧: commit id (hash)는 6자리면 충분할 것 같아요!

jin-Pro · 2022-09-13T00:11:18Z

와,, 정말 많이 배우고 갑니다.. 훌륭하세요 !!!

hanseul-lee · 2022-09-13T02:25:44Z

build 에러 수정해 올려놓았습니다(a0f597c)
작은 것들이라 혹시 린트 설정 자동으로 고치게 제대로 되어있지 않다면 참고 - autofix 부분 확인해 주시면 도움 되실 거 같아 남겨드립니다~

wherehows added 11 commits September 12, 2022 18:45

chore: add time file in utils folder

08d5bd7

chore: add path alias in webpack config

fd3c7df

feat(view): add additional information in detail component

919a030

feat(view): change style of detail summary

4ad24a4

feat(view): change style of divider

9585fa1

feat(view): add collapsible marker

bde9696

feat(view): changed fixed height of detail

7271096

feat(view): add visibility to summary

5907153

fix(view): fix that read more btn doesnt work

a2dd04b

chore: resolve conflict

23eefa1

feat(view): change color for visibility

697bac4

wherehows added the ✨ view label Sep 12, 2022

wherehows added this to the v0.1.0 milestone Sep 12, 2022

wherehows requested a review from a team as a code owner September 12, 2022 14:56

wherehows self-assigned this Sep 12, 2022

hanseul-lee reviewed Sep 12, 2022

View reviewed changes

jejecrunch approved these changes Sep 12, 2022

View reviewed changes

hanseul-lee changed the title ~~feat(view): Vertical Cluster View Issue 반영~~ feat(view): fix verticalClusterList issues and refactor detail component Sep 12, 2022

vgihan approved these changes Sep 12, 2022

View reviewed changes

ytaek approved these changes Sep 12, 2022

View reviewed changes

jin-Pro approved these changes Sep 13, 2022

View reviewed changes

wherehows merged commit 26fa6fc into githru:main Sep 13, 2022

hanseul-lee mentioned this pull request Sep 13, 2022

style(view): fix style issues in detail component #157

Merged

vgihan mentioned this pull request Sep 13, 2022

fix(view): modify variable height ClusterGraph transition & update scroll #159

Merged

wherehows mentioned this pull request Oct 5, 2022

chore(view): install dayjs library #255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(view): fix verticalClusterList issues and refactor detail component #154

feat(view): fix verticalClusterList issues and refactor detail component #154

wherehows commented Sep 12, 2022 •

edited by hanseul-lee

Loading

hanseul-lee Sep 12, 2022

wherehows Sep 12, 2022 •

edited

Loading

hanseul-lee Sep 12, 2022 •

edited

Loading

wherehows Sep 12, 2022

hanseul-lee Sep 12, 2022

hanseul-lee Sep 12, 2022

jejecrunch Sep 12, 2022

hanseul-lee commented Sep 12, 2022

jejecrunch left a comment

jejecrunch Sep 12, 2022

jejecrunch Sep 12, 2022

wherehows Sep 13, 2022 •

edited

Loading

jejecrunch Sep 14, 2022

jejecrunch commented Sep 12, 2022

jejecrunch commented Sep 12, 2022

vgihan left a comment •

edited by hanseul-lee

Loading

vgihan Sep 12, 2022

vgihan Sep 12, 2022

wherehows Sep 14, 2022

vgihan Sep 12, 2022

wherehows Sep 14, 2022 •

edited

Loading

hanseul-lee commented Sep 12, 2022

ytaek left a comment

jin-Pro commented Sep 13, 2022

hanseul-lee commented Sep 13, 2022

feat(view): fix verticalClusterList issues and refactor detail component #154

feat(view): fix verticalClusterList issues and refactor detail component #154

Conversation

wherehows commented Sep 12, 2022 • edited by hanseul-lee Loading

Related Issue

📌 Key Changes

1. 시간을 보여주기 위한 함수 및 폴더 추가(08d5bd7)

2. detail 컴포넌트 추가적인 정보 추가(919a030)

3. collapsible 을 판단할 수 있는 마커 추가 (bde9696)

4. authors, commits, changed files, changed line of codes에 대한 정보 간략화 (4ad24a4)

5. 가시성 향상 (5907153)

6. Detail 컴포넌트 고정된 높이에서 가변 높이로 변경 (7271096)

📌 To Reviwers

Choose a reason for hiding this comment

wherehows Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

hanseul-lee Sep 12, 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

hanseul-lee commented Sep 12, 2022

jejecrunch 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 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jejecrunch commented Sep 12, 2022

jejecrunch commented Sep 12, 2022

vgihan left a comment • edited by hanseul-lee 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

wherehows Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

hanseul-lee commented Sep 12, 2022

ytaek left a comment

Choose a reason for hiding this comment

jin-Pro commented Sep 13, 2022

hanseul-lee commented Sep 13, 2022

wherehows commented Sep 12, 2022 •

edited by hanseul-lee

Loading

wherehows Sep 12, 2022 •

edited

Loading

hanseul-lee Sep 12, 2022 •

edited

Loading

wherehows Sep 13, 2022 •

edited

Loading

vgihan left a comment •

edited by hanseul-lee

Loading

wherehows Sep 14, 2022 •

edited

Loading