-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: 칸반 보드 페이지에서 지원자의 지원 상태를 카드에 보여준다. #197
Conversation
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.
엄청 빠르게 작업하셨네요..! 수고하셨습니다!
@@ -17,6 +17,7 @@ export type KanbanCardData = { | |||
heart: number; | |||
isHearted: boolean; | |||
applicantId: string; | |||
passState: "non-passed" | "first-passed" | "final-passed"; |
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.
passState의 타입이 계속 반복될 수 있을 것 같은데,
이를 따로 타입으로 정의하는 것이 좋을 수도 있을 것 같습니다!
const KanbanCardApplicantStatusLabel = ({ | ||
passState, | ||
}: KanbanCardApplicantStatusLabelProps) => { | ||
switch (passState) { |
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.
div와 여러 class값들이 반복되는 것 같습니다!
위와 같은 경우 변경되는 부분을 상수로 정의해두고 가져오는 방법을 사용해도 좋을 것 같습니다!
예를 들면 아래처럼 사용할 수 있을 것 같습니다.
const labelConfig = {
'non-passed': { backgroundColor: 'bg-gray-300', label: '처리중' },
'first-passed': { backgroundColor: 'bg-lime-300', label: '1차 합격' },
'final-passed': { backgroundColor: 'bg-blue-300', label: '최종 합격' },
};
// 컴포넌트 내부
const { backgroundColor, label } = statusConfig[passState];
return (
<div className={`${backgroundColor} text-xs px-[10px] py-[5px] rounded-[10px]`}>
{label}
</div>
);
추가적으로 이와 유사한 내용을 담은 아티클이 있어서 공유합니다!
https://velog.io/@eunbinn/dry-the-common-source-of-bad-abstractions
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.
어.... 사실 같이 코딩하지 않는 사람으로서 죄송합니다...
그래도 이 프로젝트를 만들었던 사람으로서 먼가 제 생각을 추가적으로 덧 붙여 적습니다..
그냥 공감해주셔도 좋고, 이렇게도 할 수 있구나로 넘기셔도 좋습니다.(꼭 적용 안하셔도 좋고, 오히려 반박을 하면 더욱 좋습니다.)
@@ -48,12 +50,15 @@ function KanbanCardComponent({ | |||
return ( | |||
<div | |||
className={cn( | |||
"border-[1px] w-[14.9rem] p-3 rounded-lg drop-shadow-md bg-white hover:border-primary-400", | |||
"border-[1px] w-[14.9rem] p-3 rounded-lg drop-shadow-md bg-white hover:border-primary-400 relative", |
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.
relative는 어떤 작업을 하나요?
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.
아 이걸 안지웠네요. 감사합니다 !
지원자의 상태를 나타내는 라벨을 position: absolute로 해서 처음에 작업하였는데 지원자의 이름이 길어지면 라벨이 이름을 덮는 경우가 생겨서 이 방법으로 하지 않고 전공과 라벨을 형제로 만들어서 부모에 flex를 적용하는 방법으로 변경하였습니다.
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.
좋습니다!! 만약에 이름이 길어진다고 하면, tailwind에서 text-ellipsis
의 옵션을 사용하셔도 좋을 것 같습니다.
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.
오 좋네요 !
제가 tailwind를 처음 써봐서 아직 아는게 별로 없네요 ..
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.
쓰면서 차차 늘어가실 거에요... 사실 css에 있는 기능이라 그것을 공부하고 tailwind를 찾는 것을 추천 들려요
<div className="text-xs text-secondary-200 flex justify-between items-center"> | ||
{major} | ||
<KanbanCardApplicantStatusLabel passState={passState} /> | ||
</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.
오... 이거 고치기 힘들 었을 것 같은데 잘 하셨군요 ㅠㅠ
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.
제 흔적이 이렇게 흘러가는 군요...
저번 학기에 V3로 가는 문서에서 보면 (권한이 없다면 요청해주세요!!) component라는 postfix는 빼는 것이 좋아보입니다.
사실 이번 프로젝트에서는 어떻게 이루어지는지 잘 몰라서 그냥 첨언하고 갑니다 ㅠㅠ
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.
V3로 가는 문서가 어떤것인가요?
component는 제거하겠습니다 !
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 { backgroundColor, label } = labelConfig[passState]; | ||
return ( | ||
<div | ||
className={`${backgroundColor} text-xs px-[10px] py-[5px] rounded-[10px]`} |
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.
recruit에코노 홈페이지에서 css는 px를 잘 쓰지 않습니다.
px말고 다른 고정된 값이 있는지 살펴보시고 tailwind에서 주어지는 것을 적극적으로 활용하면 좋을 것 같습니다.
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.
추가적으로 tailwind에서는 classname을 이어 붙일 때 위와 같이 template literal로 처리하는 것도 좋지만, 더 안정성 있게 사용하기 위해서 classnames, clsx등을 사용하여 class결합을 도와주는 라이브러리를 적극적으로 사용하는 것이 tailwind를 사용하는데 있어 더 안전성 있게 사용할 수 있습니다.
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.
저도 제 프로젝트에서 classnames를 사용해왔었는데 안전성이 있게 사용할 수 있어서 사용한다기 보다는 복잡한 클래스를 간단하게 사용할 수 있어서 사용했었는데 혹시 안전성이 높아진다는게 어떤 의미인지 자세히 알려주실 수 있을까요?
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.
예를 들어 사용하지 않는 class나, 중복되는 class일시, 제거 또는 최 후방의 class들을 선택하도록 덮어씌우는 역활을 합니다.
그러기 때문에 만약의 상태에서도 css가 깨지지 않거나, 이상한 값이 들어가는 것을 방지해줍니다!
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 labelConfig = { | ||
"non-passed": { | ||
backgroundColor: "bg-gray-300", | ||
label: "처리중", | ||
}, | ||
"first-passed": { | ||
backgroundColor: "bg-lime-300", | ||
label: "1차 합격", | ||
}, | ||
"final-passed": { | ||
backgroundColor: "bg-blue-300", | ||
label: "최종 합격", | ||
}, | ||
}; |
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.
매우 좋은 접근인 것 같습니다. for가 아닌 object로 접근하는 것은 일관된 성능을 보장하여 더욱 사용자에게 잘 보여줄 수 있을 것입니다.
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을 강력하게 적용하는 것입니다. 예를 들어 다음과 같습니다.
interface labelConfigType {
"backgroundColor": string,
"label": 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.
밑에 읽고나서 전체적으로 보았을 때 이 곳에 이 label에 대한 object가 있는 것이 적절하지 않다고 생각합니다.
제 생각으로는 constant에서 타입을 생성하고, 이 object를 관리하는 것이 좋다고 생각합니다.
과도한 분리라고 생각할 수 있느나, 오히려 ts를 사용하기 위해서 export를 중구난방으로 사용하고, 이를 활용하는 곳이 3군데 이상일 뿐 아니라 다른 도메인인 이자 layer인 store의 수준까지 가야하기 때문에 이 component에 있는 것이 오히려 store를 해쳐 추후 확장성 또는 관리하는데 있어 힘들 수 있다고 생각합니다.
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.
혹시 첫번째 comment에서 for가 아닌 object로 접근하는 것이 좋다는 것에서 for가 어떤것인지 알 수 있을까요?
반복문의 for를 말하는건가요?
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.
네네 array를 통해 for를 접근하는 것을 말했어요! 이렇게 되면 선형적이지만, object라면 각 키라서 일정한 성능을 기대할 수 있거든요!!
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.
저같은 경우에는 constant부분에 새롭게 파일명을 생성하여 다음과 같이 진행할 것 같습니다.
const labelConfig: Record<ApplicantPassState, labelConfigType> = {
"non-processed": {
backgroundColor: "bg-gray-300",
label: "처리중",
},
"first-passed": {
backgroundColor: "bg-lime-300",
label: "1차 합격",
},
"final-passed": {
backgroundColor: "bg-blue-300",
label: "최종 합격",
},
"non-passed": {
backgroundColor: "bg-red-300",
label: "탈락",
},
};
interface labelConfigType {
backgroundColor: string;
label: string;
}
export type ApplicantPassState =
"non-processed" | "non-passed" | "first-passed" | "final-passed";
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.
확실히 Record<string, labelConfigType> 보다는 Record<ApplicantPassState, labelConfigType> 이 방식이 더 좋아보이네요 !
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.
그러면 한가지 궁금한 점이 있는데 ApplicantPassState를 이렇게 변경하는게 더 좋은 방식인 것 같나요??
export interface ApplicantPassState {
passState: keyof typeof labelConfig;
}
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.
제 생각에는 큰 상관은 없으나, 주요 메인이 누구인지, key들을 많이 사용하는지 데이터들을 주요해서 쓰는지 본다면, key들이 고정값들로 쓰일 것 같아 오히려 위와 같은 방식이 더 좋아보입니다.
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.
누가 메인인지를 생각해보면 알 수 있는 문제이네요. 답변 감사합니다 !
frontend/src/apis/kanban/index.ts
Outdated
export interface ApplicantPassState { | ||
passState: "non-passed" | "first-passed" | "final-passed"; | ||
} |
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.
여기에 타입이 있기에는 실질적으로 CardApplicantStatusLabel.component.tsx
에 연결되어 있습니다.
즉 여기에 이 함수가 있는 것이 적절하지 않다고 생각합니다. 하지만 밑에 사용하고 있으므로 이를 적절하게 사용하고 있는 곳에서 가지고 오는 것을 추천드립니다. 예를 들어 다음과 같이 할 수 있습니다.
export type ApplicantPassState ={
passState: keyof labelConfig
}
주요 변경사항
리뷰어에게...
슬랙에서 이야기 나왔듯이 지원 상태가 하나 더 추가될 예정이어서 백엔드 측에서 지원자의 상태가 추가되면 Type을 추가하고 KanbanCardApplicantStatusLabel 컴포넌트에 case 문이 하나 더 추가될 예정입니다.
관련 이슈
closes #196
체크리스트
reviewers
설정label
설정