-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enhance(view): Detail Component Contents #88
Conversation
{committer.emails.map((email: string, i: number) => ( | ||
<p key={i}>{email}</p> | ||
{commitNodeListInCluster.map((commitNode) => ( | ||
<div key={commitNode.commit.id}>{commitNode.commit.message}</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- li tag로 변경해야 할 것 같아요
- commitNode를 구조분해할당하지 않은 이유가, 어떻게 확장될지 정해지지 않아서 이렇게 사용했습니다!
// ); | ||
// console.log(parentCommits); | ||
const commitNodeListInCluster = flatCommitNodeList.filter( | ||
(commitNode) => commitNode.taskId === taskId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
({taskId}) => taskId === taskId
가 작동하지 않아 분해할당하지 않았습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
원래는 중복된 변수명도 작동해야하나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
원래는 중복된 변수명도 작동해야하나요?
아뇨 ㅎㅎㅎ 나름의 코드 작성에 대한 의견을 작성했습니다 ㅎㅎ
PR이나 commit title에, 동사(verb)를 하나 넣어주시면 더 이해하기 편할 것 같아요~. 더했다, 고쳤다, 뺐다 이런거? |
if (data?.length === 0) return undefined; | ||
const flatCommitNode: CommitNode[] = data | ||
type GetCommitListInCluster = GlobalProps & { taskId: number }; | ||
export const getCommitListInCluster = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저희가 클릭하면 통계쪽도 바뀌는 시스템인데 통계쪽에서는 현재 taskId가 아닌 데이터로 전달받고 있어서 이 부분 합쳐지면 유용하게 쓰일 것 같습니다 ! 일단 우선 저는 디테일에 clusterId인 taskId를 전달하는 걸 먼저 구현해보겠습니다 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 저도 맞춰서 진행하겠습니다!
2d0cf08
to
9aeb880
Compare
enhance(view): merge fix(view): fix value naming taskId => ClusterId enhance(view): add Cluster Detail Data in Detail Component
const diffStatistics = commitNodeListInCluster.reduce( | ||
(acc, { commit: { diffStatistics: cur } }) => ({ | ||
insertions: acc.insertions + cur.insertions, | ||
deletions: acc.deletions + cur.deletions, | ||
}), | ||
{ | ||
insertions: 0, | ||
deletions: 0, | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분 코드 너무 좋은 거 같아요! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사드립니다. 이부분에서 조금 더 고민했던 내용이
{
insertions: 0,
deletions: 0,
}
이 부분을 상수로 분리할까? 라는 고민을 했었습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다 ~! 😁
commit: { | ||
author: { names }, | ||
}, | ||
}: CommitNode) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 lint에서 줄바꿈이 많이 일어나면, 구조 분해 할당을 적게 쓰거나 안쓰는 편인데 아래와 같이 적용하는 것은 어떨까요 ? 사실 아래처럼 사용하면 names를 참조하기 위해 프로퍼티를 더 깊게 명시해야하는 단점이 있지만 줄바꿈을 덜 일으킬 수 있을 것 같습니다 !!
const fn = (set: Set<unknown>) => ({ commit }: CommitNode) => {
commit.author.names.forEach((name) => set.add(name));
return getDataSetSize(commitNodeListInCluster, fn);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 ,, 좋은 거 같아요 다른분들 의견도 궁금하네요 ㅎㅎㅎ 줄바꿈이 너무 많아서 저도 고민했었거든요
const getDataSetSize = <T extends any[]>(arr: T, callback: Function) => { | ||
const set = new Set(); | ||
const fn = callback(set); | ||
arr.forEach(fn); | ||
return set.size; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(사소) callback이 달려있는데, getDataSetSize
라는 이름만 보면, callback을 기대하지 못할 것 같습니다. 좋은 이름이 있으면 바꿔주는 것도 괜찮아 보이네용.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 의견 감사드립니다!!
네이밍은 언제나 어려운것 같습니다.. ㅜㅜ
commitNodeListInCluster, | ||
}: GetCommitListAuthorLength) => { | ||
const fn = | ||
(set: Set<unknown>) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(궁금) 혹시 Set<unknown>
을 써야하는 이유가 있을지요?
자료들의 type들이 왠만큼 정해진 상태인 것 같은데, 명시해줄 수 있거나 컴파일러가 추론해줄 수 있는 형태로 간다면 좀 더 코드가 단단해질 수 있지 않을까 해서 여쭤봅니다~.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vscode에서 type 추론된 타입이 Set이였는데 ,, 사실 저도 작성하고 나니 불편했었습니다..
type 관련해서 더 확인해보고 commit 남기겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 그러면 generic을 쓰던지 해도 괜찮을 수 있겠네요~.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 부분 string으로 fn 코드 내부에서만 선언해주면 되더라구요!
- 9a13f79
로 수정해서 commit 올렸습니다!
}; | ||
type ObjAddCommarReturn = { [key in keyof ObjAddCommarProps]: string }; | ||
const objAddCommar = (obj: ObjAddCommarProps): ObjAddCommarReturn => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(궁금) objAddCommar는 무슨 뜻인지요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obj안에 존재하는 value에 ','를 넣는 함수입니다..!
네이밍이 많이 부실한것 같군요 ㅜㅜ
ex) {
money : 1000
} => {
money : '1,000'
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아... 그러면 typo 같네요 :-) comma
ex) {
money : 1000
} => {
money : '1,000'
}
그런데, 이건 어떨 때 사용하나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numAddCommar 함수가 세 자리씩 끊어서 콤마만 붙여주는 역할만 한다면, toLocaleString 함수를 사용하는 것은 어떨까요 ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와 ,, 지금 확인했는데 ,,, ㅋㅋㅋㅋ 엄청나네요,, 역시 내장함수 많이 알아두는게 정말 중요한것 같아요,,,
수정해서 올리겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저의 피같은 numAddCommar 코드를 지우고 toLocaleString으로 변경했습니다 ㅎㅎ 감사합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앜ㅋㅋㅋ 코드 도둑돼버렸네요,, 반영 감사합니다 !!
import type { GlobalProps, CommitNode } from "types/";
export const getCommitListInCluster = ({
data,
clusterId,
}: GlobalProps & { clusterId: number }) => {
return data
.map((clusterNode) => clusterNode.commitNodeList)
.flat()
.filter((commitNode) => commitNode.clusterId === clusterId);
};
const getDataSetSize = <T extends any[]>(arr: T) => {
const set = new Set(arr);
return set.size;
};
const getCommitListAuthorLength = (commitNodes: CommitNode[]) => {
return getDataSetSize(commitNodes.map((d) => d.commit.author.names).flat());
};
const getChangeFileLength = (commitNodes: CommitNode[]) => {
return getDataSetSize(
commitNodes.map((d) => Object.keys(d.commit.diffStatistics.files)).flat()
);
};
export const getCommitListDetail = (commitNodes: CommitNode[]) => {
const authorLength = getCommitListAuthorLength(commitNodes);
const fileLength = getChangeFileLength(commitNodes);
const insertions = commitNodes.reduce(
(acc, cur) => acc + cur.commit.diffStatistics.insertions,
0
);
const deletions = commitNodes.reduce(
(acc, cur) => acc + cur.commit.diffStatistics.deletions,
0
);
return {
authorLength,
fileLength,
commitLength: commitNodes.length,
insertions,
deletions,
};
}; 올려주신 유틸 코드라인수를 줄여봤고.. 테스트는 안해서 안돌아갈수도 있긴합니다. 필요하시면 참고해주세요! |
type ObjAddCommarProps = { | ||
authorLength: number; | ||
fileLength: number; | ||
commitLength: number; | ||
insertions: number; | ||
deletions: number; | ||
}; | ||
type ObjAddCommarReturn = { [key in keyof ObjAddCommarProps]: string }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type 관련한 것들 따로 Detail.type.ts 파일로 빼주시면 한눈에 파악하기 좋을 것 같습니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵!!
WorkList
Before this commit, Detail Components contents showed the contents of one target commit, but after this commit, it shows the commit list in cluster
Result
Todo
Add WorkList
[ fix(view): fix value naming taskId => ClusterId ]
[ enhance(view): add Cluster Detail Data in Detail Component ]