-
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
[#69] Feat: 짤 상세 모달 get api 구현 및 적용 #108
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 [isLiked, setIsLiked] = useState(imageLikeYn); | ||
const { zzalDetails, isPending } = useGetZzalDetails(IMAGEID); | ||
|
||
if (isPending || !zzalDetails) 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.
<Fragment> <Fragment />
로 변경해주시면 좋을듯요 !
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.
현진님 고생 많으셨습니다 👍
변수명,함수명 위주로 살펴봐봤습니다!
@@ -83,13 +65,13 @@ const ImageDetailModal = ({ isOpen, onClose }: Props) => { | |||
Icon={FolderDown} | |||
iconLabel="다운로드" | |||
children="다운로드" | |||
onClick={handleDownloadZzal} | |||
onClick={handleDownloadButton} |
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.
handleDownloadButton
도 다른 핸들러 함수 이름과 동일하게 handleClickDownloadButton
으로 변경하는 것은 어떤가요?
{ | ||
/*TODO: [2024.03.05] 해당 handler함수 로직 추가하기*/ | ||
} | ||
const handleClickCopyButton = debounce(() => { |
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.
현진님 궁금증 하나 남겨봅니다 😀
debounce를 써서 더블클릭 등을 했을 때 여러 번 이벤트가 발생하는 것을 방지해줄 수 있군요! 배워갑니다 👍
상세 페이지에 있는 신고, 삭제 버튼에도 debounce를 걸어주는게 좋을까요? 아니면 불필요할까요?
삭제 시에도 debounce를 걸어주는 것이 좋다는 글을 하나 봐서 여쭤봅니다!
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.
저는 신고하기는 모달이 바로 나와서 괜찮을 것 같은데 삭제시에는 debounce를 걸어주시면 좋을 것 같습니당👍
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.
메인에서 pull해서 삭제 함수에 debounce 적용했습니다~!
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.
@HY219 삭제 시에 debounce를 걸어주는게 왜 좋은지 여쭤봐도 될까요?
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.
얼핏 봤던 글이였는데 제가 잘못 알았던 것 같습니다.
찾아보니까 아래와 같은 글도 있네요!
현진님 수정해주셨는데 번거롭게 해드렸네요ㅜ!
아직 언제 debounce를 사용해주는지 파악이 안된 것 같아요
https://thewavelet.tistory.com/65
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.
아 그러면 삭제시 디바운스를 제거하고 요청 중에 click하지 못하도록 변경하면 될까요?
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.
오 그렇게 해주실 수 있으면 저는 좋은 방법이라고 생각합니다! 😀
{ | ||
/*TODO: [2024.03.05] 해당 handler함수 로직 추가하기*/ | ||
} | ||
const handleClickCopyButton = debounce(() => { |
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.
@HY219 삭제 시에 debounce를 걸어주는게 왜 좋은지 여쭤봐도 될까요?
return ( | ||
<Suspense fallback={"...pending"}> | ||
<Modal isOpen={isOpen} onClose={onClose} size="sm"> | ||
<ImageDetailModalContent /> |
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.
useSuspenseQuery를 사용하기 위해 컴포넌트 상위에서 Suspense로 감싸줘야했습니다.
따라서 ImageDetailModalContent를 따로 분리했습니다.
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.
suspense 때문에 불필요한 컴포넌트 분리가 생겼는데, 이 부분이 좀 걸리네요ㅠ 저도 한 번 고민해보겠습니다
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.
처음엔 짤이 있는 페이지에서 로딩 화면 보여주다가 데이터가 다 오면 모달을 띄어주는 걸 생각했습니다.
지금 생각해 보니 스켈레톤을 넣는 것도 좋을 것 같네요!
스켈레톤을 넣는다면 상단 메뉴에 #버튼을 제외한 4개의 버튼에 각각 표현하고 아래 이미지 렌더링 되는 부분은 다 스켈레톤 처리하는 건 어떨까요? 이렇게 되면 useQuery로 구현해도 되는 걸까요?
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/utils/zzalUtils.ts
Outdated
canvas.width = image.naturalWidth; | ||
canvas.height = image.naturalHeight; | ||
|
||
const ctx = canvas.getContext("2d"); |
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.
축약어 금지 컨벤션이 있어서 네이밍 바꿔야 할 것 같아요ㅠ
canvasContext
나 imageContext
는 어떤가요? 🤔
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.
canvasContext로 변경하겠습니다!
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.
LGTM!
@@ -0,0 +1,9 @@ | |||
const Spinner = () => { |
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.
spinner의 사이즈를 조절할 수 있게 daisyUI에서 제공하는 spinner 컴포넌트 사이즈 토큰을 props로 전달받으면 어떨까요?
또 확장성을 위해 className도 props로 받고 div에 주입하는 건 어떨까요?
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.
관련해서 재민님과 논의해봤는데 현재로는 스피너가 사용되는부분이 짤 상세모달 버튼밖에 없어서
우선 패스하고 추후에 필요할시 변경하기로 했습니다:0
#69/feature/image-detail-modal-api
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.
LGTM 🦖
📝 작업 내용
추후 전송버튼에도 활용할 수 있을 것 같습니다.
close #69