Skip to content

Conversation

@99minji
Copy link
Collaborator

@99minji 99minji commented Nov 10, 2024

작업 내용

[linkbray #51]
📢 현재 link 페이지가 없어서 즐겨찾기 페이지에서 구현하였습니다. 추후 link 페이지에서 연동되게 설정하겠습니다. (11/10)
ㄴ link 페이지 merge 되어 link 페이지에 구현되도록 변경했습니다. ( 11/11)

  • 링크 수정 모달은 시안에 없어서 기존 모달 기반으로 새로 생성했습니다.
  • 링크 삭제하기 클릭 시 작업되어있는 DeleteLinkModal 사용했습니다.
  • 링크 삭제 & 수정 후 바로 반영되기 위하여 store 생성하여 관리하도록 작업했습니다.
  • 수정 & 삭제 후 렌더링 이미지 로드 때문에 성능이 느린 것 같습니다 🤔
    Screenshot 2024-11-10 at 21 04 43

@99minji 99minji linked an issue Nov 10, 2024 that may be closed by this pull request
Copy link
Collaborator

@junjeeong junjeeong left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

const LinkCard = ({
onEdit,
openDelete,
isFavoritePage,
Copy link
Collaborator

Choose a reason for hiding this comment

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

prop으로 받지 않고 const isFavoritePage = router.pathname === '/favorite'; 라는 식의 코드를 안에서 처리해주면 어떨까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네 수정하겠습니다!

onEdit,
openDelete,
isFavoritePage,
...info
Copy link
Collaborator

Choose a reason for hiding this comment

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

제가 링크 페이지 PR 올리면서 고쳐놓긴 했는데 ...info로 적어주면 링크데이터 객체 값을 하나하나 명시해주겠다는 의미인데! 그러면 LinkCard 컴포넌트를 리턴할때마다 코드가 엄청 길어집니다! map으로 돌릴때 el 요소를 그냥 info에 넣어주는게 전 더 깔끔한 것 같아요!

Copy link
Collaborator

@venise5224 venise5224 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다👍

Comment on lines 49 to 72
(isSubscribed ? (
<div
onClick={() => setIsSubscribed(!isSubscribed)}
className="absolute top-[15px] right-[15px] z-1"
>
<Image
src="/icons/star-fill.svg"
width={32}
height={32}
alt="subscripe button"
/>
</div>
) : (
<div
onClick={() => setIsSubscribed(!isSubscribed)}
className="absolute top-[15px] right-[15px] z-1"
>
<Image
src="/icons/star-empty.svg"
width={32}
height={32}
alt="subscripe button"
/>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

isSubscribded에 따라 이미지만 바뀌니
src={isSubscribed ? "/icons/star-fill.svg" : "/icons/star-empty.svg"} 한줄로 분기처리하면 더 깔끔할거 같습니다 !

Copy link
Collaborator

Choose a reason for hiding this comment

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

오 저도 이거 신경쓰였는데 이런 방법이👍👍

openDelete: () => void;
}

const Dropdown = ({ isDropdownOpen, onEdit, openDelete }: DropdownProps) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

isDropdownOpen은 어떤 용도 인가요 ?.?
따로 쓰여지고 있지는 않는거 같아서요 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

modal 열림 상태일 때 상태 변경하려고 사용했는데 useModalStore에서 isOpen 상태를 가져와 isModalOpen에 저장하도록 변경했습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

링크페이지와 즐겨찾기 페이지에서 쓰이기 위해서 store로 구현하신 건가요 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

링크 수정, 링크 삭제 후 새로고침 없이 기존 목록에 바로 반영되기 위해 store로 구현했습니다!

@99minji
Copy link
Collaborator Author

99minji commented Nov 11, 2024

코드리뷰 수정했습니다! 다시 한 번 확인 부탁드립니다!

// onClick={handleSubmit}
onClick={handleDelete}
width="w-full"
height="h-[51px] "
Copy link
Collaborator

Choose a reason for hiding this comment

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

모달 파일마다 공백이 있었더라구요 😅👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

안그래도 요걸 제가 안만들어놨었는데 추가해주셨군요!👍

@99minji 99minji merged commit 27faf78 into develop Nov 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

링크 수정, 삭제 dropdown

5 participants