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

Feature/#096 탭 브라우징 틀 구현 #101

Merged
merged 19 commits into from
Nov 13, 2024

Conversation

pipisebastian
Copy link
Member

@pipisebastian pipisebastian commented Nov 12, 2024

📝 변경 사항

🔍 변경 사항 설명

상수 분리

  • color, size, spacing등을 상수로 뺀 뒤, panda css 토큰 적용을 해줬습니다.
  • 이유는 해당 상수값들을 panda css에서만 쓰는게 아닌, framer motion과 js 코드상에서 쓰이기 때문입니다.
  • 상수로 빼면 어디서든 똑같은 값을 가져올 수 있기에, 유지보수성을 높이고자 했습니다.

framer motion 파일 분리

  • 애니메이션의 경우, 너무 길어져서 일단은 animation파일을 따로 분리했습니다!
  • 나중에 어떻게되는지 결정됨에 따라 다시 수정하겠습니다!

types 절대경로 문제

  • 연규님께서 말씀해주신 문제인데, @types 로 파일을 불러올 수 없습니다.
  • 원인은 @types 경로는 node_modules상의 @types을 가져오려하기 때문이었습니다.
  • 해결방안은 2가지가 생각나는데, 네이밍을 바꾸기보단 차라리 src/types~로 불러오는것도 나쁘지 않다고 생각했습니다!
  • 해당 방식으로 괜찮을까요? 다른분들 의견이 궁금합니당!
    • @types 대신, 네이밍 바꾸기 ex) @type, @dto,,,
    • 다른 경로로 불러오기 src/types/~

🙏 질문 사항

  • 주요로직은 코멘트 달겠습니다!
  • 타입스크립트를 제대로 사용했는지 (더 자세하게 타입을 지정할 수 있다거나, 옵셔널이 필요하지않다거나)
  • 전체적으로 컨벤션이 일치하는지
  • page 관리 로직, 드래그앤드롭, 리사이즈 관리 로직이 괜찮은지

📷 스크린샷 (선택)

2024-11-13.2.23.43.mov

✅ 작성자 체크리스트

  • Self-review: 코드가 스스로 검토됨
  • Unit tests 추가 또는 수정
  • 로컬에서 모든 기능이 정상 작동함
  • 린터 및 포맷터로 코드 정리됨
  • 의존성 업데이트 확인
  • 문서 업데이트 또는 주석 추가 (필요 시)

@pipisebastian pipisebastian self-assigned this Nov 12, 2024
@pipisebastian pipisebastian added FE 프론트엔드 작업 Feat 새로운 기능 추가나 기존 기능 확장 작업 labels Nov 12, 2024
Copy link
Collaborator

@Ludovico7 Ludovico7 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 +28 to +44
<AnimatePresence>
<motion.div
ref={pageRef}
className={pageContainer}
initial={pageAnimation.initial}
animate={pageAnimation.animate({
x: position.x,
y: position.y,
isActive,
})}
style={{
width: `${size.width}px`,
height: `${size.height}px`,
zIndex,
}}
onPointerDown={() => handlePageSelect(id)}
>
Copy link
Collaborator

Choose a reason for hiding this comment

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

AnimationPresence부분을 별도의 파일로 분리하고 내부 컴포넌트들을 children으로 관리하는 방법도 생각했었는데, 생각해보니 이 애니메이션 컴포넌트는 페이지 컴포넌트에서만 사용되는 애니메이션이라 재사용성 관련해서 큰 이득이 없을 수도 있을거 같네요. 애니메이션 로직과 비즈니스 로직을 분리한다는 관점에서는 괜찮을 수도 있을거 같은데 딱히 하나의 파일로 관리해도 문제는 없을거 같아서 이부분은 어떻게 생각하시나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

저는 개인적으로 지금방식처럼 애니메이션 코드만 딱 분리하는게 좋아보입니당!
말씀하신것처럼 비즈니스로직을 분리한다는 점은 좋지만, 직접 짜보니 애니메이션 컴포넌트가 재활용되지 않는다면, 오히려 코드의 양이 2배가 되더라구요! 이부분은 멘토님께도 여쭤봅시당~~

const INIT_ICON = "📄";
const PAGE_OFFSET = 60;

export const usePagesManage = (): usePagesManageProps => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

커스텀훅 네이밍을 ~Manage로 하셨는데 저는 ~Manager로 작성을 했습니다. 일반적으로 동사형을 많이 사용하는거 같아서 제가 작성한 커스텀훅도 동사형으로 변경하겠습니다!

Comment on lines +21 to +34
<AnimatePresence>
<nav className={navWrapper}>
{pages?.map((item) => (
<motion.div
key={item.id}
initial={animation.initial}
animate={animation.animate}
transition={animation.transition}
>
<PageItem {...item} onClick={() => handlePageSelect(item.id, true)} />
</motion.div>
))}
</nav>
</AnimatePresence>
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
Member Author

Choose a reason for hiding this comment

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

후후 이 코멘트보고 바로 구현하러갔습니다! 나중에 해당 PR도 리뷰 부탁드립니다!

…into Feature/#096_탭_브라우징_틀_구현
@@ -12,6 +12,7 @@
},
"dependencies": {
"@pandabox/panda-plugins": "^0.0.8",
"framer-motion": "^11.11.11",
Copy link
Member Author

Choose a reason for hiding this comment

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

헉 11.11.11... 의미있는 숫자군요

Comment on lines +21 to +34
<AnimatePresence>
<nav className={navWrapper}>
{pages?.map((item) => (
<motion.div
key={item.id}
initial={animation.initial}
animate={animation.animate}
transition={animation.transition}
>
<PageItem {...item} onClick={() => handlePageSelect(item.id, true)} />
</motion.div>
))}
</nav>
</AnimatePresence>
Copy link
Member Author

Choose a reason for hiding this comment

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

후후 이 코멘트보고 바로 구현하러갔습니다! 나중에 해당 PR도 리뷰 부탁드립니다!

Comment on lines +28 to +44
<AnimatePresence>
<motion.div
ref={pageRef}
className={pageContainer}
initial={pageAnimation.initial}
animate={pageAnimation.animate({
x: position.x,
y: position.y,
isActive,
})}
style={{
width: `${size.width}px`,
height: `${size.height}px`,
zIndex,
}}
onPointerDown={() => handlePageSelect(id)}
>
Copy link
Member Author

Choose a reason for hiding this comment

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

저는 개인적으로 지금방식처럼 애니메이션 코드만 딱 분리하는게 좋아보입니당!
말씀하신것처럼 비즈니스로직을 분리한다는 점은 좋지만, 직접 짜보니 애니메이션 컴포넌트가 재활용되지 않는다면, 오히려 코드의 양이 2배가 되더라구요! 이부분은 멘토님께도 여쭤봅시당~~

Copy link
Collaborator

@minjungw00 minjungw00 left a comment

Choose a reason for hiding this comment

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

정말 고생 많으셨습니다!

@github-actions github-actions bot merged commit 496c2bd into dev Nov 13, 2024
3 checks passed
SHADOW: "#004585",
RED: "#F24150",
YELLOW: "#FEA642",
GREEN: "#1BBF44",
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 content = css({
position: "relative",
width: "100%",
padding: "md",
Copy link
Collaborator

Choose a reason for hiding this comment

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

따로 자주 사용하는 px 단위를 정의하셨군요! 확인했습니다.

lg: { value: "24px" },
sm: { value: `${SPACING.SMALL}px` },
md: { value: `${SPACING.MEDIUM}px` },
lg: { value: `${SPACING.LARGE}px` },
Copy link
Collaborator

Choose a reason for hiding this comment

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

constants 활용 👍

@hyonun321
Copy link
Collaborator

틀을 정말 잘만드신 것 같습니다. 세세하게 놓칠수 있는 부분도 전부 처리해놓으셨고 방대한 양의 코드와 정확도에 박수 치고 갑니다. 정말 수고하셨습니다!

@hyonun321 hyonun321 deleted the Feature/#096_탭_브라우징_틀_구현 branch November 15, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FE 프론트엔드 작업 Feat 새로운 기능 추가나 기존 기능 확장 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants