-
Notifications
You must be signed in to change notification settings - Fork 0
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(member): 게시판 재분류, 해시태그 기능 추가 #261
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
@@ Coverage Diff @@
## main #261 +/- ##
==========================================
- Coverage 94.06% 93.66% -0.40%
==========================================
Files 14 14
Lines 219 221 +2
Branches 37 39 +2
==========================================
+ Hits 206 207 +1
- Misses 11 12 +1
Partials 2 2 |
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 list = [ | ||
{ | ||
title: '언어', | ||
items: data.filter((item) => item.hashtagCategory === 'LANGUAGE'), | ||
}, | ||
{ | ||
title: '분야', | ||
items: data.filter((item) => item.hashtagCategory === 'FIELD'), | ||
}, | ||
{ | ||
title: '기술', | ||
items: data.filter((item) => item.hashtagCategory === 'SKILL'), | ||
}, | ||
{ | ||
title: '기타', | ||
items: data.filter((item) => item.hashtagCategory === 'ETC'), | ||
}, | ||
]; |
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.
언어 - LANGUAGE, 분야 - FIELD 이런 데이터들은 매핑이 가능한 데이터들이라서, 상수로 관리했으면 더 깔끔하지 않았나 싶어요. 해당 데이터들을 상수로 관리했더라면 다음과 같이 간단하게 나타낼 수 있었을 듯 해요!
const HASHTAG = [{ titleKor: '언어', titleEng: 'LANGUAGE' }, ... ];
const list = [ | |
{ | |
title: '언어', | |
items: data.filter((item) => item.hashtagCategory === 'LANGUAGE'), | |
}, | |
{ | |
title: '분야', | |
items: data.filter((item) => item.hashtagCategory === 'FIELD'), | |
}, | |
{ | |
title: '기술', | |
items: data.filter((item) => item.hashtagCategory === 'SKILL'), | |
}, | |
{ | |
title: '기타', | |
items: data.filter((item) => item.hashtagCategory === 'ETC'), | |
}, | |
]; | |
const list = HASHTAG.map(({ titleKor, titleEng }) => ({ | |
title: titleKor, | |
items: data.filter((item) => item.hashtagCategory === titleEng), | |
})); |
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.
오 좋은 방식이네요! 해당 부분 반영해서 리팩토링 했어요 감사합니다!
/> | ||
{data.imageUrl && ( | ||
<Image | ||
height="min-h-[300px]" | ||
alt="thumbnail" | ||
className="object-cover" | ||
className="object-cover " |
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.
불필요하게 공백이 들어가는 부분들이 있는데, 해당 부분들은 prettier 설정을 통해서 보완을 하면 좋을 것 같아요
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.
알아보고 추후에 보완할 부분들 합쳐서 진행해볼게요.
@@ -64,6 +66,7 @@ export interface CommunityWriteItem { | |||
wantAnonymous: boolean; | |||
fileUrlList?: string[]; | |||
imageUrl?: string; | |||
hashtagNames?: Array<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.
한 타입 안에 string[] 과 Array이 혼재되어있는데, 하나로 통일해서 사용하는게 좋을 듯 해요
/** | ||
* 커뮤니티 게시글을 해시태그로 조회합니다. | ||
*/ | ||
export function useBoardByHashtag({ hashtags, page = 0, size = 20 }: Params) { |
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.
기본 설정값인 page와 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.
해당 부분은 별도로 코드 전체적으로 리팩토링 할 때 진행해야 할 것 같고, size
는 변경되는 경우가 많아 해당 경우마다 상수로 새로 정의할 것인가도 부분도 고민이 되네요. 추후에 의논해봐요.
setHashtagList((prevHashtag) => { | ||
const updatedHashtags = prevHashtag ? [...prevHashtag] : []; | ||
|
||
return updatedHashtags?.includes(value) |
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.
꼭 수정이 필요한 부분은 아니지만, updatedHashtags?.includes(value)
부분을 hashtagExists
와 같은 이름을 가진 변수로 빼주고 리턴하면 가독성이 더 올라갈 것 같아요!
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.
대체적으로 잘 작성해 주신 것 같고, 사소한 부분 코멘트 남겨놓았어요. 제가 리액트 쿼리 부분은 아직 알지 못하는 부분이 많아 리뷰 달기 힘든 점이 아쉽네요. 빠른 시일 내에 배워 오겠습니다. 구현 수고 많으셨어요 👍
Summary
게시판 재분류, 해시태그 기능 추가
Tasks
To Reviewer
해시태그를 직접 작성해서 추가하는 방식으로 진행하려고 했지만 백엔드의 보안공격표면을 넓힐 수 있다는 의견이 있어서 테이블로 관리하며 해시태그 조회하는 API를 통해 가져오는 걸로 결정했어요.
Screenshot