feat(view): Detail Component #72

jin-Pro · 2022-09-03T05:15:51Z

Related Issue

close [View] The message property does not exist in the commit type. #70

Work List

enhance(view): add Commit Type Message Property closed [[View] The message property does not exist in the commit type. #70]
commit type에 message 추가하였습니다.
enhance(view): CommitNode Type add taskId Property
commitNode Type에 taskId추가하였습니다.
taskId를 통해 동일 Cluster 내부 Commit 을 찾을 수 있는데, Detail Component에 해당 Commit들을 나타내줘도 좋을 것 같지만, 무슨 의미가 있나 싶긴한데 얘기를 나눠보고 싶어요.
혹시 몰라 추가했습니다.
feat(view): Detail Component
scss 작성하지 않고 해당 ui template만 작성해서 commit 남겼습니다.

enhance(view): CommitNode Type add taskId Property feat(view): Detail Component

jin-Pro · 2022-09-03T05:17:13Z

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

 import type { GlobalProps } from "types/global";

+import { getTargetCommit, getTime } from "./Detail.util";
+
+const TARGET_ID = "2a7a93cde9c9f74d5f05c1d0fb1da8e96da7057b";


Detail.const.ts로 분리하지 않은 이유가, App.tsx에서 Props로 commit Id를 받을 것인지, 아니면 해당 commit 자체를 받을것인지 논의를 해야할 것 같아 이렇게 작성하였씁니다!

ytaek

좋습니다!!

ytaek · 2022-09-03T16:10:57Z

packages/view/src/components/Detail/Detail.util.ts

+
+import type { CommitNode } from "types/NodeTypes.temp";
+
+type GetTargetCommit = GlobalProps & { id: string };


Data를 담는 용도의 Type이라면 Get(동사) 보다는 어떤 데이터인지 표현하는게 좋은 것 같습니다.

getTargetCommit이라는 함수의 type을 선언한 부분이라, 함수 명은 camel case, type 명은 pascal case 로 작성한 부분입니다.

그럼에도 func 의 type을 데이터에 맞춰서 이름을 맞추는게 나을까요??
아니면 func의 naming 부터 수정해야하는걸까요???

의견 주셔서 감사드립니다!

ytaek · 2022-09-03T16:12:49Z

packages/view/src/components/Detail/Detail.util.ts

+  if (data?.length === 0) return undefined;
+  const flatCommitNode: CommitNode[] = data
+    .map((clusterNode) => clusterNode.commitNodeList)
+    .flat();


(궁금) commitNodeList가 CommitNode[] 인 것 같은데, flat은 어떤 이유로 붙이셨을까요?

flat을 사용하지 않는 경우
data.map((clusterNode) => clusterNode.commitNodeList)는 2차원 배열로 만들어져서
해당 일차원 배열을 돌면서, 내부 배열을 순회하여
target commit을 찾아야하는

번거로움이 존재해서 flat으로 펼치고, 바로 filter로 찾기 위함이였습니다!

ytaek · 2022-09-03T16:14:28Z

packages/view/src/types/NodeTypes.temp.ts

@@ -41,7 +41,7 @@ export type Commit = {
  authorDate: Date;
  commitDate: Date;
  diffStatistics: DiffStatistics;
-
+  message: string;


빠진 것 채워주셔서 감사합니다 :)

ytaek · 2022-09-03T16:15:39Z

packages/view/src/types/NodeTypes.temp.ts

@@ -63,7 +63,7 @@ export type CommitNode = NodeBase & {
  nodeTypeName: "COMMIT";
  commit: Commit;
  seq: number;
-
+  taskId: number; // 동일한 Cluster 내부 commit 참조 id


말씀드렸듯이, taskId가 clusternode의 id가 될 것 같습니다.
정확한 이름 등은 engine 팀 쪽을 참조 부탁드립니다.

hanseul-lee

수고하셨습니다~👍

hanseul-lee · 2022-09-03T15:45:51Z

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

+
+      <div>Committer Name</div>
+      {committer.names.map((name: string, i: number) => (
+        <p key={i}>{name}</p>


(궁금) 왜 key값으로 index를 사용하셨는지 궁금합니다.

아 이부분,,, 급하게 작성하다보니 index로 해버렸네요,, 의도치못한 코드입니다 ㅜㅜ merge이후 수정해보겠습니다!

hanseul-lee · 2022-09-03T15:45:55Z

packages/view/src/components/Detail/Detail.util.ts

+type GetTargetCommit = GlobalProps & { id: string };
+
+export const getTargetCommit = ({ data, id }: GetTargetCommit) => {
+  if (data?.length === 0) return undefined;


이부분 d3관련 제약이 있는게 아니라면 명시적으로 undefined를 return하기보다 null을 사용하시는 건 어떠신가요?
#14 (reply in thread)

(잡담)
저는 이게 javascript 자체가 다른 언어랑 다르게 undefined, null, NuN, 등등 여러가지가 있어서 생기는 문제 중의 하나라고 보는데요.
예를 들어 그냥 java 등등이라면 'null'이라는 것만 신경쓰면 되는데, js 는 그렇지가 않은거죠..

그래서 js/ts 에 익숙하지 않거나, 여러 언어를 다루는(ployglot?) 저같은 개발자들은 걔들을 하나로 통일하고 싶어할 때도 있습니다. 헷갈리거든요 null, undefined, 등등... ㅋ
큰 문제가 없다면 undefined로 가는게 더 나을 수도 있어요. npm package 들에도 null을 안 쓰는 놈들도 많을 꺼구요.

아, undefined로 고정하자! 뭐 이런 얘기는 아니었구요. (모든 것에는 tradeoff가 있을꺼기에.. ㅎㅎ)
어찌됐건, 하나의 프로젝트 안에서는 rule이 있고, 그게 약간 명확한게 좋긴 하니까 정하는 것도 괜찮고,
토론을 통해서 정해지면 후루룩 다 바꾸는 것도 괜찮겠습니다 : -)

사실은 eslint : consistent-return 발생해서 넣었습니다,,,,
코드를 급하게 작성한감이 있었는데, getTargetCommit 내부에서 data.length 체크해주는 것보다 위 함수를 호출하는 로직에서 data.length를 체크해주는게 좀 더 명확한 함수일 것이란 생각을 했습니다.

이 부분도 리펙토링때 수정하겠습니다.

저는 getTargetCommit 함수가 커밋을 찾는 함수이기에 찾을 수 없다면 null을 사용하는게 더 어울리다고 생각합니다.. 찾지 못했으니 없는거니까요!

의견 감사드립니다!

hanseul-lee · 2022-09-03T15:46:37Z

packages/view/src/components/Detail/Detail.util.ts

+
+import type { CommitNode } from "types/NodeTypes.temp";
+
+type GetTargetCommit = GlobalProps & { id: string };


오~ 이런 식으로 type을 사용할 수 있군요~! 새롭게 배워갑니다👍👍👍

hanseul-lee · 2022-09-03T16:27:01Z

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

+      <p>{message}</p>
+
+      <div>Committer Name</div>
+      {committer.names.map((name: string, i: number) => (


author와 committer 중 왜 committer를 보여주려는지 여쭤봐도 될까요?
현재 VerticalClusterList 와 Statistics에서는 author 데이터를 주로 사용하는 것으로 알고 있습니다.

추가로 만약 author를 사용한다면 현재 엔진팀에서 논의된 바로는 co-author를 고려하지 말자고 결론난 것으로 알고 있어 author 배열에 값이 하나만 담기게 되어 map을 돌리지 않고 바로 꺼내올 수 있을 것 같습니다~!

cf. author vs committer 차이

사실 Detail Component가 보여줄 데이터가 정말 없습니다..
VerticalClusterList/Summary에서는 Message와 author를
Statistics에서는 diff를 나타내주시다보니
작성할 내용이 없어서,,, 어거지라도 넣어본게 committer였습니다 ㅜㅜ

wherehows · 2022-09-04T01:59:21Z

packages/view/src/components/Detail/Detail.util.ts

+    .map((clusterNode) => clusterNode.commitNodeList)
+    .flat();
+  const [target] = flatCommitNode.filter(
+    (commitNode) => commitNode.commit.id === id


Suggested change

(commitNode) => commitNode.commit.id === id

({ commit } ) => commit.id === id

요런식으로 사용하는 것에 대해서는 어떻게 생각하시나요? @_@

오 좋은 것 같습니다.
둘의 장단점은 있을 것 같아요.

윗쪽은 변수명을 넣으면서 어떤 용도로 사용 되는지에 대한 표기를 할 수 있을 꺼고
아랫쪽은 코드가 짧아지고 간결해지겠네요 : -)

맞습니다 ㅎㅎ 저도 디스트럭쳐링할당 정말 좋아하는데요, 간과했던것 같습니다 ㅎㅎ

wherehows · 2022-09-04T02:14:55Z

packages/view/src/components/Detail/Detail.util.ts

+};
+
+export const getTime = (date: Date) =>
+  String(date).split(".")[0].replace("T", " ");


utc 타임에서 z만 제거하는게 아니라 local time으로 바꿔주어야 할것 같은데, 어떻게 생각하시나용?!! @_@
FYI: https://bobbyhadz.com/blog/javascript-convert-utc-to-local-time

넵 ㅎㅎ 다른분들 의견도 들어보고싶습니다 해당부분 discussion에 올려주실수있을까요??

enhance(view): add Commit Type Message Property closed [githru#70]

e7f5bec

enhance(view): CommitNode Type add taskId Property feat(view): Detail Component

jin-Pro added the ✨ view label Sep 3, 2022

jin-Pro self-assigned this Sep 3, 2022

jin-Pro requested a review from a team as a code owner September 3, 2022 05:15

jin-Pro commented Sep 3, 2022

View reviewed changes

ytaek approved these changes Sep 3, 2022

View reviewed changes

hanseul-lee approved these changes Sep 3, 2022

View reviewed changes

wherehows reviewed Sep 4, 2022

View reviewed changes

jin-Pro merged commit 6068366 into githru:main Sep 4, 2022

hanseul-lee added this to the v0.1.0 milestone Sep 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(view): Detail Component #72

feat(view): Detail Component #72

jin-Pro commented Sep 3, 2022

jin-Pro Sep 3, 2022

ytaek left a comment

ytaek Sep 3, 2022

jin-Pro Sep 4, 2022

ytaek Sep 3, 2022

jin-Pro Sep 4, 2022

ytaek Sep 3, 2022

ytaek Sep 3, 2022

hanseul-lee left a comment

hanseul-lee Sep 3, 2022

jin-Pro Sep 4, 2022

hanseul-lee Sep 3, 2022 •

edited

Loading

ytaek Sep 4, 2022

jin-Pro Sep 4, 2022

hanseul-lee Sep 3, 2022

hanseul-lee Sep 3, 2022 •

edited

Loading

jin-Pro Sep 4, 2022

wherehows Sep 4, 2022

ytaek Sep 4, 2022 •

edited

Loading

jin-Pro Sep 4, 2022

wherehows Sep 4, 2022 •

edited

Loading

jin-Pro Sep 4, 2022


		import type { CommitNode } from "types/NodeTypes.temp";

		type GetTargetCommit = GlobalProps & { id: string };

	(commitNode) => commitNode.commit.id === id
	({ commit } ) => commit.id === id

feat(view): Detail Component #72

feat(view): Detail Component #72

Conversation

jin-Pro commented Sep 3, 2022

Related Issue

Work List

Choose a reason for hiding this comment

ytaek 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

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

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

hanseul-lee Sep 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ytaek Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wherehows Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanseul-lee Sep 3, 2022 •

edited

Loading

hanseul-lee Sep 3, 2022 •

edited

Loading

ytaek Sep 4, 2022 •

edited

Loading

wherehows Sep 4, 2022 •

edited

Loading