Skip to content
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 #871

Open
wants to merge 8 commits into
base: part2-김도현
Choose a base branch
from

Conversation

DoDOhyeon
Copy link

요구사항

기본

  • TypeScript를 활용해 프로젝트에 타입 명시가 필요한 곳을 찾아 타입을 명시했나요?
  • 링크 검색바에 검색어를 입력하면 현재 폴더에 있는 링크들 중 “url”, “title”, “description” 중 하나에 검색어가 포함된 링크들만 필터된 상태로 볼 수 있나요?
  • 링크 검색바에 입력 값이 있는 경우 x 버튼이 보이나요?
  • 링크 검색바에 x 버튼을 누르면 입력값이 없던 UI 상태로 돌아가나요?

심화

  • 상단에 있던 링크 추가하기 영역이 가려져 보이지 않을 때 최하단에 링크 추가하기 영역을 고정하도록 만들었나요?
  • 푸터가 시작되는 지점에서는 최하단에 고정된 링크 추가하기 영역이 보이지 않도록 했나요?

주요 변경사항

  • 링크 추가하기 input이 화면에서 가려질 때 최하단 고정
  • 타입스크립트 적용하여 타입 지

스크린샷

image

멘토에게

-타입 지정 시 오류가 나는 부분 몇 개는 any로 지정하였습니다. 이번주내로 수정하겠습니다.

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@DoDOhyeon DoDOhyeon requested a review from lisarnjs March 27, 2024 04:16
@DoDOhyeon DoDOhyeon added the 순한맛🐑  마음이 많이 여립니다.. label Mar 27, 2024
@DoDOhyeon DoDOhyeon closed this Mar 27, 2024
@DoDOhyeon DoDOhyeon reopened this Mar 28, 2024
Copy link
Collaborator

@lisarnjs lisarnjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 위클리 미션 열심히 해서 제출해주셨군요!!
제가 코드리뷰가 좀 늦어서 죄송합니다 ㅜㅜ
전체적으로 너무 어렵지 않은 내용들 먼저 코멘트 달아놨어요!
이해 안되는거 있으면 여쭤보세요 :)

중복되는 내용은 한번만 작성했으니 중복되는 부분 발견하시면 같이 수정해주시면 됩니당

이번주도 고생 많으셨어요! 멘토링 시간에 뵈요 👍


return (
<BackGround ref={ref} $isAddLinkVisible={$isAddLinkVisible}>
{!$isAddLinkVisible ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$변수가 styled-component용이 아닌 상태변수였군요..!
jquery스러운 방식이라 실제로 컴포넌트 내부에서는 잘 사용하지 않긴 합니다 :)


const Menus = ({ changeTitle, changeID, setIsVisible }) => {
const listsData: any = useGetPromise(getFolderList);
const lists = listsData?.data ?? [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?? 연산자를 쓰셨군용 gooood 👍

<a href={url} target="_blank" rel="noreferrer">
<Folder onMouseOver={handleMouseOver} onMouseOut={handleMouseOut}>
<ImageContainer>
{img_src ? (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위에 img_src를 빈 string값으로 초기화 시켜줬다면 항상 img_src가 true인 상태로 판단될거에요!
즉, 삼항연산자의 의미가 없다는 뜻입니다 ㅜㅜ! 의도한게 아니라면img_src !== '' ? :요렇게 바꿔보시는게 어떨까요?!

padding="16px 20px"
fontSize="16px"
radius="8px"
onBtnHandle={() => {}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오잉 빈 함수를 보내주고 있네요?! 아직 추가가 안된걸까요??

}

export const SharedModal = ({ $isModalVisible, setIsModalVisible }) => {
const ICONS = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 상수화 시켜주는 거 너무 잘했습니다 :)

padding="10px 20px"
fontSize="18px"
radius="8px"
onBtnHandle={() => {}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

호옥시 빈 함수가 의도한거라면 ()=>null 로 보내주는 것이 더 깔끔할 것 같아요!
null을 바로 return할 수 있도록


export const useGetPromise = (func) => {
const [values, setValues] = useState([]);
const HandleLoad = useCallback(async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

함수명은 보통 카멜 케이스를 사용합니다! 파스칼케이스는 컴포넌트명에 사용되기 때문에 혼동이 올 수 있어요!

setValues(results);
}, []);

useEffect(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleLoad()가 useEffect()안에서 또 호출되고 있죠!?
handleLoad()에 useCallback으로 넣어주는 대신 useEffect안에서 바로 함수 내용 코드를 작성해보는 것을 추천드릴게요!

observer2.observe(footerDivRef.current);
}

return () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useEffect에서 observer를 clean up 해줄 때 추천하는 한가지 방식은 아래와 같아요!

return () => { 
  items.forEach(item => {
    observer.unobserve(item)
  })
  observer.disconnect()
}

unobserve()와 disconnect()를 모두 사용해주는 것인데요
unobserve()는 특정 요소에 대한 가시성 변화 주시를 중단하고, disconnect()는 모든 가시성 변화 주시 대상을 해제하는 것이죠

즉, 우리가 disconnect()만 사용한다면 DOM에서 몇 개의 요소는 여전히 옵저버가 걸려있는 상태로 남아있을 수 있어요. 확신할 수 없다는 뜻이죠

반대로 unobserve()만 사용한다면 내가 모든 요소의 옵저버 연결을 끊어줬다고 해도 observer는 여전히 사용될 가능성이 있다는 뜻이기에 메모리에 남아있게 되고 이는 곧 메모리 낭비 + 퍼포먼스에 불필요한 영향을 끼치게 될 수 있어요

그래서 가장 확실하게 옵저버를 clean up 했다고 보장할 수 있는 방법으로 위 코드를 사용한답니다
DOM에서 완벽하게 지워주는 것입니다 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
순한맛🐑  마음이 많이 여립니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants