feat(view) add Summary in VerticalClusterList Component #63

jejecrunch · 2022-09-01T15:46:10Z

Recent Issue

#40

WorkList

4c79264

key value 때문에 @pshdev1030 님께서 언급해주신 React 18에서 도입된 useId()를 사용해보려고 했는데 :r00: 과 같이 return 되어 same key라고 자꾸 에러가 뜨더라고요 ㅜㅜ @hanseul-lee 님께서 언급해주신 nanoid 라이브러리를 추가하여 해결하였습니다.

3e84dc3

VerticalClusterList 우측에 존재하는 Summary 컴포넌트를 추가하였습니다.
VerticalClusterList 팀의 경우 이용하기 쉽게 다음와 같은 interface로 데이터를 가공하여 저장하고 있습니다.

953dd59

Summary 내에서만 관리하다가 Content 컴포넌트로 분리해보았습니다.
이후에 Author와 Message 컴포넌트로 나눌 예정입니다.

99047ca

nanoid 버전 업데이트 (npm install을 하는 바람에 ^^;;..)

2dcb82b

run fail 에러 수정 -> npm run build 추가했습니다.

978b2d5

코멘트 받은 내용을 토대로 수정한 내용입니다.

scss 관련하여 mixin, include 사용
Content 컴포넌트 삭제
type 지정하면서 보니 전역 Commit 타입에 message 빠진 거 추가

Result

(변경 전)

(변경 후)

Summary 부분에 Summary가 아닌 우선 클러스터별 전체 Commit List가 출력되기로 결정하였습니다.
이후 시간적 여유가 있다면 기존에 구현했던 Summary 추가해서 현재 전체 Commit List가 출력되는 화면을 다른 컴포넌트로 빼고 Summary 클릭 이벤트 발생 시에 전체 Commit List가 나오도록 하는 것도 구현해보고 싶습니다.
자세한 내용은 #40 에서 확인 부탁드립니다.

Questions

View 팀 회의에서 type or interface를 이야기했을 때 interface를 하자고 얘기가 되었는데 파일명을 Summary.type.ts로 남겨도 괜찮을까요? Summary.interface.ts로 바꾸어야 할까요?
color의 경우 정해진 게 없어서 @hanseul-lee 님께서 말해주신 color로 우선 대체하였습니다. main color 1가지, sub color 3가지 정도 논의해보면 좋을 것 같습니다. -> 현재 기존에 있는 팔레트 이용하여 색 넣었습니다.

After To do

~~- [ ] Detail 컴포넌트에 boolean state value 전달할 수 있게 논의하면서 수정~~
~~- [ ] Author Name Circle을 겹쳐보이게 변경~~

머지한 부분 싱크 맞추기

jejecrunch · 2022-09-01T16:02:00Z

@all-contributors please add jejecrunch for code

allcontributors · 2022-09-01T16:02:09Z

@jejecrunch

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

wherehows · 2022-09-02T04:28:04Z

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

+	-moz-transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+	transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+
+    -webkit-box-shadow: 0px 0px 3px 1px rgba(50, 50, 50, 0.4);


0에 px이 붙어있는 곳도 있고, 안 붙어 있는 곳도 있는데, 혹시 하나로 통일하는 것은 어떨까요!

(설마 특정 스타일 속성은 생략 못하거나 그런게... 있나요? ㄷ_ㄷ)

피드백 감사합니다 ! 다른 분들과 아직 얘기하기 전이어서 우선 0으로 설정했는데 혹시 0과 0px이 다를까요? 다른 분들과도 얘기해보는 게 좋을 것 같아요 !

다르지 않은데 일관성 측면에서 말씀드렸습니다 ㅎ_ㅎ! 💪

wherehows · 2022-09-02T04:29:01Z

packages/view/package.json

@@ -84,6 +84,7 @@
    "@testing-library/react": "^13.3.0",
    "@testing-library/user-event": "^13.5.0",
    "d3": "^7.6.1",
+    "nanoid": "^4.0.0",


nanoid 추가 댔군요!!

다른분들도 알고 계시면 좋을 듯 하네용. 이미 논의된 내용이라면 죄송합니다. 🙏

저는 uuid가 익숙해서 왜 nanoId를 사용하셨을까 궁금해서 nanoid vs uuid 키워드로 구글링해봤는데

https://blog.bitsrc.io/why-is-nanoid-replacing-uuid-1b5100e62ed2

여기에 설명이 나와있네요!

star 수가 uuid보다 nanoid가 더욱 많고,
뿐만아니라, nanoid에서 더 작은 데이터로 uuid를 대체할 수 있으며,
nanoid가 uuid보다 크기가 적다고하네요!

wherehows · 2022-09-02T04:29:58Z

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

+    justify-content: center;
+    align-items: center;
+    text-align: center;
+    font-size: 15pt;


pt 단위는 또 처음보네용. 하나 배워갑니닷. 🙏

(궁금) 혹시 pt와 px을 혼합해 사용하시는 이유가 있을까요?

저는 font-size의 경우는 pt를 쓰는 게 조금 더 익숙해서 ㅎㅎ ,, (아니면 em) 혹시 단위와 관련해서 조금 더 얘기 나눠보면 좋을 것 같습니다. 특히 font-size의 경우 따로 지정된 값이 없었기 때문에 임의로 지정한 거여서요 !

wherehows · 2022-09-02T04:39:13Z

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

+      {info.names.map((v, i) => {
+        return (
+          <p className="summary" key={`${nanoid()}`}>
+            <span className="nameBox" key={`${nanoid()}`}>


키는 순회해서 반환하는 jsx 태그의 가장 바깥쪽 태그에만 할당해도 되지 않나용?

key에 nanoid, uuid와 같은 함수를 매번 호출해서 넣어주는 방식은 안티 패턴인 것으로 알고 있어요 ! List를 렌더링 할 때 변경이 일어난 부분을 key prop을 통해 감지하게 되는데, 렌더링마다 매번 다른 key 값이 들어가게 되면 List의 전체 element들을 다시 렌더링 한다고 알고 있습니다...! 그래서 미리 nanoid를 데이터에 부여해두고 변경되지 않는 값으로 사용하거나, 다른 key 값을 사용해야할 것 같습니다 !

그런데 다시 생각해보면 현재는 리렌더링이 일어날 상황이 info 데이터의 변경뿐이고, 그렇게 되면 결국에는 모든 List의 렌더링이 다시 일어날 것 같아서 상관이 없을 것 같긴한데, 한 번 얘기 나눠보면 좋을 것 같습니다 !

nanoid 같은 경우 전체 코드가 아니라 지혜님께서 랜덤으로 함수를 만들어 사용하는 부분만 보고 추천드렸는데
데이터에 따라 더 나은 값으로 대체할 수 있는지 논의해보면 좋을 거 같습니다~

키는 순회해서 반환하는 jsx 태그의 가장 바깥쪽 태그에만 할당해도 되지 않나용?

맞습니다! p 태그에만 적용해주면 될 것 같아요!
혹시 다른 이유가 있었는지 궁금하네요!

nanoid, uuid와 같은 함수를 매번 호출해서 넣어주

저도 외부에서 key값을 만들고 사용해야한다는 기한님 의견에 동감합니다!
리렌더링 발생상황이 미미하다 하더라도 추후 어떻게 확장될지 모르니 예방하는 것도 좋아보입니다!

앗 관련하여 답변을 지금 드리네요 ㅎㅎ ... 제가 방금 의견을 들어서 해당 내용으로 진행했는데 단순히 바깥으로 뺀다고 해결될 일이 아닌 것 같습니다. 특히 map으로 감싸기 떄문에 map을 안에서 nanoid()를 실행하거나 데이터를 재가공해야할 것 같습니다 ...
리렌더링을 따로 생각하지 않은 건 아닌데 계속 에러가 떠서 ㅜㅜ 우선 급하게 PR을 올렸네요 ...

현재 상황

p태그만 했을 경우 -> 아래에 있는 span에도 모두 키를 주라는 에러 발생

nanoid()를 return 밖으로 뺏을 경우 -> 같은 아이디라는 에러 발생

해결 방안

기존 데이터에 유일성 있는 id를 각 데이터마다 부여하는 가공 프로세스를 거친다 -> 타입 변경 필요

map을 사용하기 때문에 map 외부에서 nanoid()를 사용하면 동일한 키 값이라는 에러가 발생하는 군요!

해당 이슈를 듣고나니 바로 떠오르는 생각은 왜 nanoid로 고유 id 를 만들까 였습니다!

summary.tsx 코드에서는 ids,names,messages를 따로따로 만들어주는데 ids를 사용하는 구간을 발견하지 못했는데요,
nanoid보다 names나 messages의 key값으로 해당 commit의 id를 추출한 ids에 인덱스로 접근해서 사용하는 것은 어떨까요??

사실 인덱스로 접근하는것을 좋아하지는 않지만, names와 messages를 만들때 객체로 value와 key(commit.id)값을 넣어서 객체로 만들어 주고 객체의 id를 key값으로 사용하는게 좋지 않을까? 라는 의견을 제시해봅니다!

info 객체에 ids,messages,names를 각각의 배열로 만들지 않고
info = {
id,
name,
message
} 로 만드는 것은 안될까요??

p태그만 했을 경우 -> 아래에 있는 span에도 모두 키를 주라는 에러 발생

(궁금) 이 부분이 왜 그런지 알아보고싶네요 ㅎㅎ,,,

p태그만 했을 경우 -> 아래에 있는 span에도 모두 키를 주라는 에러 발생

(궁금) 이 부분이 왜 그런지 알아보고싶네요 ㅎㅎ,,,

아래 span에도 map이 걸려있어서, 둘 다 줘야 하는거네요.
여기에 굳이 uuid/nanoid까지 써야할지는 조금 고민을 더 해보면 좋겠습니다.

윗쪽 info.names loop 안에서, info.messages가 도는 부분도 좀 어색해 보이는데,
가능하시면 자료 구조를 좀 더 다듬어도 좋을 것 같구요.
id로 쓸 commit id나 clusternode의 id등을 넘겨줘서 위의 map의 key로도 쓸 수 있을 것 같습니다.

wherehows · 2022-09-02T04:41:04Z

너무 고생하셨습니당 🙏

wherehows · 2022-09-02T04:45:51Z

packages/view/src/components/VerticalClusterList/Summary/Summary.const.ts

@@ -0,0 +1,7 @@
+import type { Info } from "./Summary.type";
+
+export const information: Info = {


제제님은 보통 타입 이름과 변수 이름을 다르게 가져가시나요? 아니면 혹시 깃스루 컨벤션일까요? 👀

저도 통일하면 좋을 거 같습니다~

추가로

const에 담긴 값들은 주로 관습처럼 대문자로 네이밍하고 있고 기한님께서도 비슷하게 작성하신 거 같은데 이부분 통일되면 좋을 거 같습니다~

const는 주로 변하지 않는 상수를 담는 곳인데 information을 이곳에 두는 것이 맞을지 궁금합니다.

저도 이 부분은 Information으로 작성하면 좋을 것 같다 생각합니다!

넵넵 그렇게 수정하겠습니다 ! 따로 얘기가 없어서 우선 이렇게 작업했습니다 !

어떤 것에 대한 information인지 명확하게 표시하면 좋을 듯 합니다.
구체적인 목적이 단어(합성어?)면 더 좋겠습니다.

vgihan · 2022-09-02T09:06:57Z

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

+      {info.names.map((v, i) => {
+        return (
+          <p className="summary" key={`${nanoid()}`}>
+            <span className="nameBox" key={`${nanoid()}`}>


key에 nanoid, uuid와 같은 함수를 매번 호출해서 넣어주는 방식은 안티 패턴인 것으로 알고 있어요 ! List를 렌더링 할 때 변경이 일어난 부분을 key prop을 통해 감지하게 되는데, 렌더링마다 매번 다른 key 값이 들어가게 되면 List의 전체 element들을 다시 렌더링 한다고 알고 있습니다...! 그래서 미리 nanoid를 데이터에 부여해두고 변경되지 않는 값으로 사용하거나, 다른 key 값을 사용해야할 것 같습니다 !

그런데 다시 생각해보면 현재는 리렌더링이 일어날 상황이 info 데이터의 변경뿐이고, 그렇게 되면 결국에는 모든 List의 렌더링이 다시 일어날 것 같아서 상관이 없을 것 같긴한데, 한 번 얘기 나눠보면 좋을 것 같습니다 !

vgihan

고생하셨습니다 !!! 😀😀

hanseul-lee · 2022-09-02T09:37:37Z

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

+.name:after {
+    -webkit-transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+	-moz-transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+	transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+}
+
+.name:hover {
+    display: flex;
+    width: 40px;
+    height: 40px;
+    border-radius: 50px;
+    font-weight: bold;
+    background-color: #0077aa;
+    color: #ffffff;
+    justify-content: center;
+    align-items: center;
+    text-align: center;
+    font-size: 15pt;
+    margin: 0 0 5px 0;
+}


이 부분들 .name 안에 &:after, &:hover 로 넣는 게 더 좋을 것 같습니다. scss를 사용하는 이유기도 하구요!

피드백 감사합니다 ! 공부하면서 하고 있다보니 이런 내용을 놓치네요 ㅜㅜ 우선 PR 먼저 올리기로 약속해서 PR 부터 올리다보니 ㅎㅎ 수정하겠습니다 ~!

저도 배워갑니당 ㅎ_ㅎ 👍

hanseul-lee · 2022-09-02T09:38:52Z

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

+    justify-content: center;
+    align-items: center;
+    text-align: center;
+    font-size: 15pt;


(궁금) 혹시 pt와 px을 혼합해 사용하시는 이유가 있을까요?

hanseul-lee · 2022-09-02T09:39:52Z

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

+    -webkit-border-radius: 5px;
+	-moz-border-radius: 5px;
+	border-radius: 5px;
+
+	content: attr(data-tooltip-text);
+
+    position: absolute;
+    top: 90%;
+	left: -9999px;
+
+
+	color: #FFFFFF;


중간에 공백이 있거나 띄워져있는 부분 prettier 적용이 안되는 것일까요?ㅜㅜ 깔끔하게 맞춰지면 더 좋을 거 같습니다~

scss가 prettier 적용이 안되어있는 것 같습니다... 해당 부분 수정하겠습니다. 앗 다시 확인해보니 개행이 2번 되었네요 ^^;;;;;;

hanseul-lee · 2022-09-02T09:42:28Z

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

+      {info.names.map((v, i) => {
+        return (
+          <p className="summary" key={`${nanoid()}`}>
+            <span className="nameBox" key={`${nanoid()}`}>


nanoid 같은 경우 전체 코드가 아니라 지혜님께서 랜덤으로 함수를 만들어 사용하는 부분만 보고 추천드렸는데
데이터에 따라 더 나은 값으로 대체할 수 있는지 논의해보면 좋을 거 같습니다~

hanseul-lee · 2022-09-02T09:43:04Z

packages/view/src/components/VerticalClusterList/Summary/Summary.const.ts

@@ -0,0 +1,7 @@
+import type { Info } from "./Summary.type";
+
+export const information: Info = {


저도 통일하면 좋을 거 같습니다~

추가로

const에 담긴 값들은 주로 관습처럼 대문자로 네이밍하고 있고 기한님께서도 비슷하게 작성하신 거 같은데 이부분 통일되면 좋을 거 같습니다~

const는 주로 변하지 않는 상수를 담는 곳인데 information을 이곳에 두는 것이 맞을지 궁금합니다.

hanseul-lee · 2022-09-02T09:47:59Z

packages/view/src/components/VerticalClusterList/Summary/Summary.type.ts

+export interface Info {
+  ids: Array<Array<string>>;
+  names: Array<Array<string>>;
+  messages: Array<string>;
+}
+
+export interface IInfo {
+  info: Info;
+}


(궁금) IInfo의 뜻은 무엇인가요?

hanseul-lee · 2022-09-02T10:05:06Z

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

+    -webkit-transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+	-moz-transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+	transition: bottom .3s ease-in-out, opacity .3s ease-in-out;


👍👍👍
오 생각도 못했는데 호환성까지 세심하게 다 고려해주셨네요!

hanseul-lee · 2022-09-02T10:07:43Z

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

+import {
+  information,
+  getCommitAuthorNames,
+  getCommitIds,
+  getCommitMessages,
+} from ".";


information은 const이고 나머지는 util에서 가져온 것을 나눠 작성해 주시면 더 좋을 거 같은 데 어떠신가요?

import { information } from "."; import { getCommitAuthorNames, getCommitIds, getCommitMessages} from ".";

를 말씀하시는게 맞을까요?? @hanseul-lee

// index.ts export { default as Summary } from './Summary; export type { Summary } from './Summary.type; // 생략 가능 // Summary.tsx export { information } from "./Summary.const"; export { getCommitIds, getCommitAuthorNames, getCommitMessages, } from "./Summary.util"; // ...

위와 같습니다.
추가로 저 information이 const에 담겨야 할 지 좀 더 얘기해 보면 좋을 거 같습니다🧐

hanseul-lee · 2022-09-02T10:13:52Z

packages/view/src/components/VerticalClusterList/Summary/index.ts

+export * as Summary from "./Summary";
+export type { Info } from "./Summary.type";
+export { information } from "./Summary.const";
+export {
+  getCommitIds,
+  getCommitAuthorNames,
+  getCommitMessages,
+} from "./Summary.util";


util과 const는 추가로 export 해주지 않아도 될 것 같습니다.
위에 Summary export { default as Summary } from './Summary 로 작성하시면 다른 곳에서 더 깔끔하게 가져다 쓸 수 있습니다~!

hanseul-lee · 2022-09-02T10:14:56Z

packages/view/src/components/VerticalClusterList/Summary/Summary.util.ts

+export function getCommitAuthorNames({ data }: GlobalProps) {
+  return data
+    .map((v) => {
+      return v.commitNodeList.map((a: CommitNode) => {
+        return a.commit.author.names
+          .map((b: string) => {
+            return b.trim();
+          })
+          .join("");
+      });
+    })
+    .map((v) => v.filter((e: string[], i: number) => v.indexOf(e) === i));
+}


혹시 v, a, b 대신 좀 더 명확하게 표현해 줄 네이밍이 있다면 그렇게 바꿔주시면 훨씬 이해하기 쉬울 것 같습니다~

ex

// AS-IS nodes.map(v => v + 1); // TO-BE nodes.map(node => node + 1);

jin-Pro · 2022-09-02T10:15:50Z

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

+    -webkit-transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+	-moz-transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+	transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+}


혹시 위 코드는 mix-in으로 만들 수 있을까요??
위 코드가 브라우저별 css를 동등하게 적용해주기 위함이라 생각하는데,
외부에서 재사용을 하지 않아도 동등성을 지원해주기 위함이니 재사용을 할 수 있다면 적용하면 좋을 것 같습니다..!!

jin-Pro · 2022-09-02T10:16:59Z

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

+      {info.names.map((v, i) => {
+        return (
+          <p className="summary" key={`${nanoid()}`}>
+            <span className="nameBox" key={`${nanoid()}`}>


키는 순회해서 반환하는 jsx 태그의 가장 바깥쪽 태그에만 할당해도 되지 않나용?

맞습니다! p 태그에만 적용해주면 될 것 같아요!
혹시 다른 이유가 있었는지 궁금하네요!

jin-Pro · 2022-09-02T10:18:32Z

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

+      {info.names.map((v, i) => {
+        return (
+          <p className="summary" key={`${nanoid()}`}>
+            <span className="nameBox" key={`${nanoid()}`}>


nanoid, uuid와 같은 함수를 매번 호출해서 넣어주

저도 외부에서 key값을 만들고 사용해야한다는 기한님 의견에 동감합니다!
리렌더링 발생상황이 미미하다 하더라도 추후 어떻게 확장될지 모르니 예방하는 것도 좋아보입니다!

jin-Pro · 2022-09-02T10:19:36Z

packages/view/src/components/VerticalClusterList/Summary/Summary.const.ts

@@ -0,0 +1,7 @@
+import type { Info } from "./Summary.type";
+
+export const information: Info = {


저도 이 부분은 Information으로 작성하면 좋을 것 같다 생각합니다!

jin-Pro · 2022-09-02T10:21:05Z

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

+import {
+  information,
+  getCommitAuthorNames,
+  getCommitIds,
+  getCommitMessages,
+} from ".";


import { information } from "."; import { getCommitAuthorNames, getCommitIds, getCommitMessages} from ".";

를 말씀하시는게 맞을까요?? @hanseul-lee

jin-Pro · 2022-09-02T10:23:56Z

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

+const Summary = ({ data }: GlobalProps) => {
+  information.ids = getCommitIds({ data });
+  information.names = getCommitAuthorNames({ data });
+  information.messages = getCommitMessages({ data });
+
+  return <Content info={information} />;
+};
+


저도 @hanseul-lee 님 의견처럼 presentational container pattern 보다는 합치는게 좋아보입니다!

jin-Pro · 2022-09-02T10:43:53Z

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

+const Summary = ({ data }: GlobalProps) => {
+  information.ids = getCommitIds({ data });
+  information.names = getCommitAuthorNames({ data });
+  information.messages = getCommitMessages({ data });


information이 불변성을 적용하지 않는 것 같은데 의도한 내용이실까요???

information이 불변성을 유지하지는 않아도 data 값의 변경에 따라 내부 객체 ids,names,messages의 값이 변경되어 렌더링에는 지장이 없겠네요..!

jin-Pro · 2022-09-02T18:59:05Z

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

+const Summary = ({ data }: GlobalProps) => {
+  information.ids = getCommitIds({ data });
+  information.names = getCommitAuthorNames({ data });
+  information.messages = getCommitMessages({ data });


information이 불변성을 유지하지는 않아도 data 값의 변경에 따라 내부 객체 ids,names,messages의 값이 변경되어 렌더링에는 지장이 없겠네요..!

jin-Pro · 2022-09-02T19:01:33Z

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

+import type { IInfo } from "../Summary.type";
+import "./Content.scss";
+
+export const Content = ({ info }: IInfo) => {


props로 info를 전달해주어 IInfo가 생긴것 같습니다!
혹시

// in Summary.tsx <Content {...information} />;

// in Content.tsx export const Content = ({ names,ids,messages }: Info) => {

로 작성가능하지 않을까? 생각해봅니다 ㅎㅎ

jin-Pro · 2022-09-02T19:54:59Z

packages/view/src/components/VerticalClusterList/Summary/Summary.util.ts

+
+export function getCommitMessages({ data }: GlobalProps) {
+  return data.map((v) => {
+    return v.commitNodeList[v.commitNodeList.length - 1].commit.message;


#40 (comment)

여기에 해당 로직 관련 질문 작성하였습니다!

kmin-jeong

nanoid 추가된거 확인했습니다!

jin-Pro · 2022-09-04T04:27:13Z

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

+    -webkit-transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+    -moz-transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+    transition: bottom .3s ease-in-out, opacity .3s ease-in-out;
+}


ㅎㅎ 훌륭하십니다!

jin-Pro · 2022-09-04T04:39:25Z

packages/view/src/components/VerticalClusterList/Summary/Summary.util.ts

+    return cluster;
+  });
+
+  return clusters;


js에서 선언형 프로그래밍 대신 명령형 프로그래밍을 작성하기 위해 사용되는 방법 중 하나가 고차함수라고 생각합니다.
고차함수는 순수함수라는 특징으로 새로운 객체를 반환해주는 불변성을 유지하는 함수라는 특징이 있는 것으로 기억하는데요,

현재 코드는 고차함수를 사용하지만 선언형 코드로 작성되어있으며, 새로운 객체를 반환해주는 값을 사용하지 않는 것 같아서
좀 어색하다는 느낌이 듭니다!

오히려 고차함수보다는 for문을 사용하거나

commitNode.commit.author.names.map((name) => { const author: Author = { id: nanoid(), name: name.trim(), }; authors.push(author); return author; }); const temp = { commitId: commitNode.commit.id, authorNames: authors, message: commitNode.commit.message, };

위 로직을

const temp = { commitId: commitNode.commit.id, authorNames: commitNode.commit.author.names.map((name) => ({ id: nanoid(), name: name.trim(), }); message: commitNode.commit.message, };

와 같이 고차함수를 사용해 반환되는 객체를 사용하는 건 어떨찌 의견을 내봅니다.

다른 분들 의견도 궁금합니다!

아래 코드가 매우 간결하고 직관적인 것 같습니다 :-) 👍
map이 주는 return을 사용하는게 좋은 것 같아요.

고차함수가 뭔지 기억이 잘 안나서 ^^; 찾아봤는데,
https://en.wikipedia.org/wiki/Higher-order_function

고차함수는 순수함수라는 특징으로 새로운 객체를 반환해주는 불변성을 유지하는 함수라는 특징이 있는 것으로 기억하는데요,

함수를 인자로 받고, return이 가능하다고는 있지만, 불변성 보장에 대한 내용은 없는 것 같은데용.
고차함수가 Functional Programming(FP)를 지향을 하는 방법 중 하나일 수는 있지만, 직접적인 인과관계가 있다라고 보기에는 좀 어렵지 않나 싶은데요.
개인적으로는 js 자체가 불변성과 FP와 친한 언어도 아니라고 보긴 합니다;;
(array의 sort만 봐도 parameter로 들어온 배열을 막 맘대로 바꾸니까요 ㅜ.)

심도있는 comment들 덕분에 오늘도 많이 배워 갑니다 ㅎㅎ

ytaek · 2022-09-04T08:12:49Z

packages/view/src/components/VerticalClusterList/Summary/Summary.util.ts

+    return cluster;
+  });
+
+  return clusters;


아래 코드가 매우 간결하고 직관적인 것 같습니다 :-) 👍
map이 주는 return을 사용하는게 좋은 것 같아요.

고차함수가 뭔지 기억이 잘 안나서 ^^; 찾아봤는데,
https://en.wikipedia.org/wiki/Higher-order_function

고차함수는 순수함수라는 특징으로 새로운 객체를 반환해주는 불변성을 유지하는 함수라는 특징이 있는 것으로 기억하는데요,

함수를 인자로 받고, return이 가능하다고는 있지만, 불변성 보장에 대한 내용은 없는 것 같은데용.
고차함수가 Functional Programming(FP)를 지향을 하는 방법 중 하나일 수는 있지만, 직접적인 인과관계가 있다라고 보기에는 좀 어렵지 않나 싶은데요.
개인적으로는 js 자체가 불변성과 FP와 친한 언어도 아니라고 보긴 합니다;;
(array의 sort만 봐도 parameter로 들어온 배열을 막 맘대로 바꾸니까요 ㅜ.)

심도있는 comment들 덕분에 오늘도 많이 배워 갑니다 ㅎㅎ

> > Co-authored-by: jejecrunch <gkswlgp456@gmail.com> Co-authored-by: vgihan <jum654@naver.com>

jejecrunch added the ✨ view label Sep 1, 2022

jejecrunch requested a review from a team as a code owner September 1, 2022 15:46

jejecrunch self-assigned this Sep 1, 2022

allcontributors bot mentioned this pull request Sep 1, 2022

docs: add jejecrunch as a contributor for code #66

Merged

wherehows reviewed Sep 2, 2022

View reviewed changes

vgihan approved these changes Sep 2, 2022

View reviewed changes

hanseul-lee reviewed Sep 2, 2022

View reviewed changes

jin-Pro reviewed Sep 2, 2022

View reviewed changes

jejecrunch force-pushed the main branch from 7c9dc0b to 3d6af47 Compare September 2, 2022 18:07

jin-Pro reviewed Sep 2, 2022

View reviewed changes

kmin-jeong approved these changes Sep 3, 2022

View reviewed changes

jejecrunch added a commit to jejecrunch/githru-vscode-ext that referenced this pull request Sep 3, 2022

refactor(view): Refactor code due to githru#63 comment and change plan

978b2d5

jejecrunch force-pushed the main branch from 3d6af47 to 978b2d5 Compare September 3, 2022 16:43

jejecrunch mentioned this pull request Sep 3, 2022

[view] Vertical Cluster List #40

Closed

2 tasks

wherehows approved these changes Sep 4, 2022

View reviewed changes

jin-Pro approved these changes Sep 4, 2022

View reviewed changes

ytaek approved these changes Sep 4, 2022

View reviewed changes

jejecrunch added 4 commits September 4, 2022 19:17

add(view): add nanoid library for key value

29dade1

feat(view): add Summary in VerticalClusterList Component

30d60f3

feat(view): add Content in Summary Component

13b7823

chore(view): Update nanoid version

5f84904

jejecrunch added 3 commits September 4, 2022 19:17

fix(view): Fix npm run build fail error

cc224df

refactor(view): Refactor code due to githru#63 comment and change plan

e418d78

fix(view): Sync graph height and commit height

678a46c

> > Co-authored-by: jejecrunch <gkswlgp456@gmail.com> Co-authored-by: vgihan <jum654@naver.com>

jejecrunch force-pushed the main branch from 978b2d5 to 678a46c Compare September 4, 2022 10:17

jejecrunch added 3 commits September 4, 2022 19:27

fix(view): Fix graph height

5e3ecc0

> > Co-authored-by: jejecrunch <gkswlgp456@gmail.com> Co-authored-by: vgihan <jum654@naver.com>

refactor(view): Apply feedback about githru#63

63d1a7c

fix(view): Fix tooltip style

fc14125

jejecrunch merged commit 8ce1ca6 into githru:main Sep 4, 2022

hanseul-lee mentioned this pull request Sep 10, 2022

feat(view): apply styles to Temporal filter #107

Merged

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

		@@ -0,0 +1,7 @@
		import type { Info } from "./Summary.type";

		export const information: Info = {

feat(view) add Summary in VerticalClusterList Component #63

feat(view) add Summary in VerticalClusterList Component #63

Conversation

jejecrunch commented Sep 1, 2022 • edited Loading

Recent Issue

WorkList

Result

Questions

After To do

jejecrunch commented Sep 1, 2022

allcontributors bot commented Sep 1, 2022

wherehows Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wherehows Sep 2, 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

wherehows Sep 2, 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

현재 상황

해결 방안

jin-Pro Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wherehows commented Sep 2, 2022

Choose a reason for hiding this comment

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

vgihan 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

jejecrunch Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

kmin-jeong 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

jejecrunch commented Sep 1, 2022 •

edited

Loading

wherehows Sep 2, 2022 •

edited

Loading

wherehows Sep 2, 2022 •

edited

Loading

wherehows Sep 2, 2022 •

edited

Loading

jin-Pro Sep 2, 2022 •

edited

Loading

hanseul-lee Sep 2, 2022 •

edited

Loading

jejecrunch Sep 2, 2022 •

edited

Loading

hanseul-lee Sep 2, 2022 •

edited

Loading