-
Notifications
You must be signed in to change notification settings - Fork 2
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: 자료집 UI 구현 #379
Feat: 자료집 UI 구현 #379
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
커밋 내역은 깃에 설정된 이메일하고 이름을 따라갑니다. git config --global user.email <내 이메일>
git config --global user.name <내 이름>
지금 올라가 있는 커밋 바꾸려면 git rebase -i 7e4c6fc -x \
"git commit --amend --author 'rail <[email protected]>' -CHEAD" 이 명령어 실행하고 force push하면 될 것 같네요. |
@EATSTEAK |
@jongse7
그냥 제가 리베이스해서 올려드릴게요. 나중에 한번 interactive rebase 공부 ㄱㄱ |
@jongse7 리베이스 해보려고 했는데 커밋이 꼬여서 그런지 계속 컨플릭트가 나네요. 커밋 히스토리를 선형으로 유지할 필요가 있어 보입니다. 머지 커밋 좀 날리고 히스토리 좀 정리할게요. 추가로 branch ruleset에 선형 히스토리 유지하도록 하고 스쿼시로 머지하도록 옵션 강제해놨으니 참고 부탁드려요. |
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.
제가 커밋 히스토리 정리해드리려 했는데 코드 보니까 컨플릭트가 그대로 남아 있네요.
develop을 어떻게 합치셨는지(머지? 리베이스?)모르겠지만 그것 때문에 코드가 다 꼬여서 원본 코드가 어떤 형태인지도 모르겠습니다.
develop 머지 전으로 되돌리시거나 다시 작업하셔야 할 듯요.
2d5aa95
to
5cc90cc
Compare
5cc90cc
to
d62ec1c
Compare
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.
이 파일 제외하고도 사용하지 않는 api 훅들이 많아 보이는데 다 삭제하죠.
getBoardDataPosts.ts
getBoardDataPostSearch.ts
getOnboardingEmail.ts
patchBoardDataPosts.ts
postBoardBoardCodeFiles.ts
postBoardDataFiles.ts
postBoardDataSubCategoryPost.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.
자료집 API인데, Lint 오류 때문에 주석 처리했어요. 제가 다시 구현해야 해서 일단 급하게 땜질했습니다
@@ -10,9 +10,18 @@ interface AuditEditImageSectionProps { | |||
export function AuditEditImageSection({ onImagesChange }: AuditEditImageSectionProps): JSX.Element { | |||
const { images, addImage, removeImage, getValidImages } = useImageManager(); | |||
|
|||
const memoizedGetValidImages = useCallback(() => getValidImages(), [getValidImages]); |
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.
해당 부분에 useCallback 로직을 추가한 이유가 궁금합니당
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.
굳이 memorized
라는 prefix를 추가할 필요가 없어 보입니다. 변수명은 해당 함수의 의미론적 동작만을 반영하도록 간결하게 작성하는 편이 좋습니다.
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.
질문 위주여서 승인합니다! 아 그리고 이전 코드리뷰 과정에서 얘기 나왔던 develop 머지 전으로 되돌려서 문제를 해결했는지도 궁금합니다..! PR 이름 기존 규칙에 맞게 수정했습니다.
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.
리뷰중이였는데 머지되었네요. 확인 부탁드려요 @jongse7
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.
이 파일 제외하고도 사용하지 않는 api 훅들이 많아 보이는데 다 삭제하죠.
getBoardDataPosts.ts
getBoardDataPostSearch.ts
getOnboardingEmail.ts
patchBoardDataPosts.ts
postBoardBoardCodeFiles.ts
postBoardDataFiles.ts
postBoardDataSubCategoryPost.ts
@@ -13,7 +13,7 @@ export async function postBoardFiles({ | |||
boardCode, | |||
files = [], | |||
images = [], | |||
}: postBoardFilesProps): Promise<AxiosResponse<any>> { | |||
}: postBoardFilesProps): Promise<AxiosResponse> { |
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.
이건 백엔드의 리턴 타입대로 구현하는게 맞을 것 같네요.
}: postBoardFilesProps): Promise<AxiosResponse> { | |
}: postBoardFilesProps): Promise<AxiosResponse<ApiResponse<UploadFilesResponse>>> { |
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.
다만 apis
아래의 훅들은 점진적으로 Deprecated 처리해야 할 것 같습니다.
@@ -11,7 +11,7 @@ export interface postBoardPostsProps { | |||
content: string; | |||
categoryCode?: string; | |||
groupCode?: string; | |||
memberCode?: string; | |||
memberCode?: string | null; |
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.
어떤 경우에 memberCode
가 null이 되나요?
그리고 백엔드에서 null로 보내준다면 ?
를 제거해야 하지 않을까요?
만약 이게 patch
같은 곳에서도 사용해야 하는 값이라면 PatchBoardPostsProps
같은 이름으로 먼저 정의하고 Required<>
Utility Type을 사용해서 다른 타입을 또 정의하는게 좋겠습니다.
); | ||
} | ||
); | ||
Button.displayName = 'Button'; | ||
|
||
export { Button, buttonVariants }; |
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.
fast-refresh
관련 이슈는 shadcn/ui 자체 이슈라 eslint-disable-next-line
으로 린트를 꺼주는게 더 적절해 보입니다. buttonVariants는 컴포넌트가 아니니까요
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.
동일하게 대문자로 만들고 export 위치 변경한것도 revert하면 좋겠습니다.
@@ -93,7 +88,6 @@ const NavigationMenuIndicator = React.forwardRef< | |||
NavigationMenuIndicator.displayName = NavigationMenuPrimitive.Indicator.displayName; | |||
|
|||
export { | |||
navigationMenuTriggerStyle, |
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.
이것도 린트를 꺼야하지 export에서 제외하면 안될 것 같습니다. react-router
의 Link
컴포넌트를 네비게이션 링크로 사용할 때에는 이 함수가 필요합니다.
className="relative h-full min-w-fit text-[20px]" | ||
onClick={() => window.open(import.meta.env.VITE_TEMP_DATA_URL, '_blank')} | ||
> | ||
<NavigationMenuItem className="relative h-full min-w-fit text-[20px]" onClick={() => navigate(dataPath)}> |
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.
이것도 Link 컴포넌트를 asChild로 넘겨주는게 좋습니다. shadcn docs 보시면 어떻게 하는지 아실 수 있어요.
const element = ref.current; | ||
if (element) { | ||
const isOverflow = element.scrollWidth > element.clientWidth || windowWidth < 1741; | ||
setIsOverflow(isOverflow); | ||
setIsOverflow(element.scrollWidth > element.clientWidth || windowWidth < 1741); |
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.
Magic value는 지양했으면 좋겠네요.
@@ -10,9 +10,18 @@ interface AuditEditImageSectionProps { | |||
export function AuditEditImageSection({ onImagesChange }: AuditEditImageSectionProps): JSX.Element { | |||
const { images, addImage, removeImage, getValidImages } = useImageManager(); | |||
|
|||
const memoizedGetValidImages = useCallback(() => getValidImages(), [getValidImages]); |
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.
굳이 memorized
라는 prefix를 추가할 필요가 없어 보입니다. 변수명은 해당 함수의 의미론적 동작만을 반영하도록 간결하게 작성하는 편이 좋습니다.
* | ||
* 자료집에 쓰이는 자료 목록에서 사용할 수 있는 자료 항목 컴포넌트입니다. | ||
* 일반적으로 `BodyLayout` 아래에 리스트 형태 아이템으로 표시할 수 있습니다. | ||
* C는 literal union이길 권장합니다만, 상황에 따라 다양한 타입을 넣을 수 있습니다. - 효민's 주석에서 발췌 |
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.
C 제네릭이 없는 것 같습니다...
1️⃣ 작업 내용 Summary
자료집 UI 제작했습니다. API 변경 여지가 있기 때문에 기존에 있던 const 데이터와 mockupData로 작업했습니다.
/data

/data/edit

/data/:id

노트북으로 보는 중인데 ArticleHeader, Container 마진 조금 줄여도 될 거 같더라고요? 어떻게 생각하시는지
(아무튼 기반을 만들어준 효민이에게 박수를...)
2️⃣ 추후 작업할 내용
3️⃣ 체크리스트
develop
브랜치의 최신 코드를pull
받았나요?