-
Notifications
You must be signed in to change notification settings - Fork 44
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
[박상준] Week13 #419
The head ref may contain hidden characters: "part3-\uBC15\uC0C1\uC900-week13"
[박상준] Week13 #419
Conversation
@sj0724 |
@o-seung-yeon |
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.
file changed 가 많아서 Next 설정과 컴포넌트 단위를 중점으로 리뷰했습니다.
전반적으로 커스텀 훅을 잘 사용하시네요.
역할을 더 분리할 순 없을지, state 를 더 줄일 순 없을지 고민하시면 더 유지보수하기 좋은 코드로 개선할 수 있을 것 같아요!
고생하셨습니다 👏
useEffect(() => { | ||
switch (id) { | ||
case 1: | ||
setSectionImage('/img_1.png'); | ||
break; | ||
case 2: | ||
setSectionImage('/img_2.png'); | ||
break; | ||
case 3: | ||
setSectionImage('/img_3.png'); | ||
break; | ||
case 4: | ||
setSectionImage('/img_4.png'); | ||
break; | ||
default: | ||
break; | ||
} | ||
}, [id]); |
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.
소개 내용을 sectionDescription 객체로 관리하고 있는데 이미지도 객체에 같이 관리하면 어떨까요?
{folderName ? folderName : '전체'} | ||
{folderId > 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.
'전체'를 선택했을 때 folderName 에는 빈 문자열, folderId 에는 0 이 설정되는 것 같네요.
의도를 명확히 표현하기 위해 불리언 변수를 하나 만들어서 사용하면 어떨까요?
const isTotal = folderName === '' && folderId === 0;
// ...
return (
// ...
{isTotal && '전체'}
{!isTotal && <버튼들 />}
// ...
};
}; | ||
}, [loading]); | ||
|
||
return ( |
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.
페이지 컴포넌트인만큼 큰 단위로 컴포넌트를 분리하면 파악하기 좋을 것 같아요.
링크 추가 / 검색 / 폴더 선택 / 카드 리스트 정도로 나눠볼 수 있을 것 같아요~
setSearchKeyWord={function ( | ||
value: React.SetStateAction<string> | ||
): void { | ||
throw new Error('Function not implemented.'); | ||
}} |
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 async function getSampleFolder() { | ||
try { | ||
const { data } = await axios.get('/sample/folder'); | ||
if (data && data.folder && data.folder.links) { |
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?.folder?.links
useEffect(() => { | ||
function handleClickOutside(e: any) { | ||
if ( | ||
kebabView && | ||
kebabRef.current && | ||
!kebabRef.current.contains(e.target) | ||
) { | ||
setKebaView(false); | ||
} | ||
} | ||
|
||
document.addEventListener('mousedown', handleClickOutside); | ||
return () => { | ||
document.removeEventListener('mousedown', handleClickOutside); | ||
}; | ||
}, [kebabView]); |
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.
kebabView 와 이 로직을 커스텀 훅으로 분리하면 어떨까요?
setFolderId: React.Dispatch<React.SetStateAction<number>>; | ||
setFolderName: React.Dispatch<React.SetStateAction<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.
onSelect: (folder: Folder) => void;
하나로 관리할 수 있을 것 같아요~
setFolderName: React.Dispatch<React.SetStateAction<string>>; | ||
setFolderId: React.Dispatch<React.SetStateAction<number>>; | ||
}) { | ||
const [linkSelected, setLinkSelected] = useState<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.
선택된 폴더 id 만 들고있으면 배열로 관리하지 않아도 될 것 같아요!
id="word" | ||
value={text} | ||
/> | ||
<S.SearchIcon src={inputImage} alt="돋보기 아이콘" /> |
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.
src 로 넘겨주는 값은 삼항 연산자로 간단하게 처리하면
inputImage 변수를 지울 수 있을 것 같아요!
|
||
const openModal = (modalName: string) => { | ||
dispatch({ type: 'OPEN_MODAL', modalName }); | ||
console.log(modalName); |
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 생성 전 로그는 지워주세요!
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게