-
Notifications
You must be signed in to change notification settings - Fork 1
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
[TASK-84, TASK-85] feat, style: 상세 페이지 우측 버튼 그룹 및 관련 모달 컴포넌트 구현 #48
base: dev
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Qodana for JS21 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked Contact Qodana teamContact us at [email protected]
|
Qodana for JS33 new problems were found
☁️ View the detailed Qodana report Contact Qodana teamContact us at [email protected]
|
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 컨벤션에 맞게, 제목에 label 추가해주세요 :)
.scroll-hide::-webkit-scrollbar { | ||
display: none; | ||
} | ||
|
||
.scroll-hide { | ||
-ms-overflow-style: none; | ||
scrollbar-width: none; | ||
} |
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.
스크롤바 숨기는 CSS를 전역에 적용하신 이유가 뭔가요?
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.
아하! 그럼 사용되는 컴포넌트 내부에서 Tailwind CSS
로 적용하시지 않고 globals.css
파일에서 적용하신 이유가 무엇인가요??
modalSize: 'sm', | ||
contents: ( | ||
<ConfirmModal | ||
otherButtonText='컬렉션으로 이돋' |
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.
otherButtonText='컬렉션으로 이돋' | |
otherButtonText='컬렉션으로 이동' |
오타 있습니다!
<ConfirmModal | ||
otherButtonText='컬렉션으로 이돋' | ||
onClickOtherButton={() => { | ||
router.push('/collections'); |
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.
router.push('/collections'); | |
router.push(ROUTE.artworkCollections); |
/artwork/collections
로 경로 상수화해서 사용하면 좋을 것 같아요!
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.
이부분은 사실 구현된게 아니다보니 우선 /collections
로 상수화 해두고 추후 콜렉션 페이지 제작 이후에 수정사항이 있으면 반영하겠습니다.
저는 /artwork/collections 보다 바뀐다면 /collections
, artist/collections
나 profile/collections
로 바뀌는게 더 나을 것 같습니다..! 현재 각 컬렉션 페이지로 가는 방법이 특정 유저 페이지 -> 컬렉션 탭 -> 컬렉션 클릭 이라 artwork 하위 폴더와는 물리적 거리가 먼 것 같은 느낌이 듭니다!
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.
아하 네네 ㅎㅎ
UI/UX 나온 거 보고 말씀드린 path가 아니여서 상수화를 제안드렸다고만 이해해주시면 좋을 것 같아용
제안주신 콜렉션 관련 경로 아이디어는 좋습니다! 나중에 해당 페이지의 UI/UX 확정되어 페이지 분담할 때 다시 논의하면 좋을 듯합니다!
const [shareURL, setShareURL] = useState(window.location.href); | ||
const [copyStatus, setCopyStatus] = useState(false); | ||
|
||
const handlOnChangeShareURL = (event: ChangeEvent<HTMLInputElement>) => { |
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 handlOnChangeShareURL = (event: ChangeEvent<HTMLInputElement>) => { | |
const handleOnChangeShareURL = (event: ChangeEvent<HTMLInputElement>) => { |
오타 있습니당
/> | ||
<span className='h-[60px]'> | ||
<SquareButtonL | ||
variant={'tertiary'} |
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.
variant={'tertiary'} | |
variant='tertiary' |
중괄호 없이 문자열로 넘기셔도 됩니다!
|
||
const { mutate: submitCreateCollection } = usePostCreateCollection(); | ||
|
||
const handlOnChangeCollectionName = ( |
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 handlOnChangeCollectionName = ( | |
const handleOnChangeCollectionName = ( |
오타 있어용
transition-all duration-300 ease-in-out active:scale-95 | ||
${disabled ? bgColorClasses['secondary'] : bgColorClasses[variant]} | ||
${disabled ? bgColorClasses['tertiary'] : bgColorClasses[variant]} |
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.
disabled
의 경우, 색상이 tertiary
로 보이는 걸로 결정된 걸까요?
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.
이게 제가 확인차 바꾼 내용이 commit에 포함된 것 같습니다..!
우선 피그마에 disabled 되어 있는 색상이 저희 디자인 내에서 활용되지 않고 있는 것 같아서... 로그인에서도 validation 통과 전 색상이 tertiary이더라구요..! 제가 전체 페이지 확인을 못해서 빼둬야지 하다가 포함된 것 같습니다..!
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.
|
||
import { authorizedClient } from '..'; | ||
|
||
const postCreateCollection = async (name: string, isPrivate: boolean) => { |
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.
타입 2개니까 따로 선언해서 부여하는 게 좋을 것 같습니다!
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 fn = ({ a, b} : { a : number, b : nubmer }) => {}
const fn = (a: number, b: number) => {}
이렇게 두가지 방식으로 코드를 짤 수 있을 것 같은데 두 개 차이가 있을까요? 단순 코드 컨벤션 차이일까요?
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.
interface FnType {
a: number,
b: nubmer,
}
const fn = ({ a, b }: FnType) => {} // 1번 방법
const fn = ( a: number, b: number ) => {} // 2번 방법
아! 네 코드 컨벤션입니다! 일단 1번 방법처럼 분리해달라고 요청드린 이유는
저번에 상우님이 제 PR 리뷰 해주실 때 타입 2개인데 분리해달라고 하셔서 논의했었잖아용
2개일 때부터 타입을 따로 선언하여 부여하는 걸로 정했기 때문에 이것도 해당 컨벤션에 맞춰 작성해드리길 요청드렸습니다!
그리고 제가 타입을 따로 분리하는 이유도 궁금하신 것 같아서 말씀드립니다.
구체적인 차이는 interface
로 작성했냐, type
으로 작성했느냐에 따라 장단점이 다르지만 저는 보통
- 가독성: 타입만 따로 보기 때문에 코드가 쉽게 읽힘
- 타입 확장성: 타입이 추후 추가될 경우, 분리해 선언해 둔 타입을 활용하여 타입을 추가할 수 있음 (
interface
의 경우,extends
로 활용 가능) - 재사용성: 한 파일 내에서 같은 타입을 참조할 경우, 여러 번 작성하여 부여할 필요가 없어서 효율적임
위 3가지 이유로 타입을 따로 분리해서 부여합니다!
const handleClickLikeToggle = () => { | ||
setBlockButton(true); | ||
toggleLike(postId); | ||
setBlockButton(false); | ||
}; |
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 handleClickLikeToggle = () => { | |
setBlockButton(true); | |
toggleLike(postId); | |
setBlockButton(false); | |
}; | |
const handleClickLikeToggle = async () => { | |
setBlockButton(true); | |
await toggleLike(postId); // 비동기 작업 대기 | |
setBlockButton(false); | |
}; |
이렇게 비동기 작업을 대기하도록 하는 건 어떻게 생각하시나여?
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.
좋습니다..! mutate가 promise 객체를 반환하고 있지 않아서 별도 비동기 처리가 필요없다고 생각하고 있었습니다..!
혹시 다른 mutate를 실행하는 함수에서는 별도의 async/await을 하지 않던데 해당 부분에서만 추가되는 특별한 이유가 있을까요? setLBlockButton을 함수 전후로 활용하고 있기 때문일까요?
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.
네, setBlockButton
이 toggleLike
함수의 앞뒤로 있기 때문이 맞습니다!
async/await
를 추가하길 제안드린 이유는 '유튜브 댓글 좋아요처럼 작동할 것을 염려해서'였습니다. 비동기 작업을 대기하지 않고 낙관적 업데이트를 진행하면 유튜브처럼 댓글 좋아요가 반영되지 않았음에도 불구하고 일단 UI 먼저 바꾸게 되면 발생할 문제가 있어서용
아직 서버에 좋아요가 반영되지 않았는데 UI는 반영된 것으로 보여서 사용자가 페이지를 이탈하거나 다른 클릭을 시도할 경우 (제가 자주 그럽니다), 좋아요 반영하는 함수 실행이 중단되기 때문에 UX상으로 불편하리라 생각합니다! 물론 유튜브는 좋아요 수가 워낙 많아서 느린 거겠지만 저는 불편했습니다!
'mutate
를 실행하는 함수에서는 별도의 async/await
을 하지 않는다'고 하셨는데 이 부분은 async/await
를 따로 개발자가 작성하지 않아도 내부적으로 실행될 코드가 동기적으로 작동하는 것이 맞는지 확인해 볼 필요가 있을 것 같아용
저는 이렇게 생각합니다 ㅎㅎ 상우님은 어떻게 생각하시나요?
📝 작업 내용
[TASK-84] - 버튼 그룹 구현
[TASK-85] - 관련 모달 구현
📸 스크린샷
우측 버튼 그룹 ( 좋아요 / 좋아요 해제 )
관련 모달
위 컬렉션 저장 모달에서 작품을 컬렉션에 저장 완료되었을 경우 나타나는 모달
특이사항
이번 PR 기준으로 리뷰 마무리되는대로 나머지 영역에 대한 코드도 차례로 이어서 PR 추가하겠습니다...!