-
Notifications
You must be signed in to change notification settings - Fork 5
[FE] feat: 길드 생성 레이아웃 구현 #35
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
Conversation
chysis
left a comment
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 본문에 대한 의견
- 파일/폴더 구조 괜찮은 것 같아요! 다만
AuthLayout.tsx의 경우엔 폴더->index.tsx의 구조가 아니라 이름이 그대로 파일명으로 들어갔는데, 이 부분도 통일하는 것은 어떨까요? - eslint rule 추가하는 것 좋아요!
- 디스코드 웹 버전으로 들어가 보시면, 로그인 후 이동하는 경로가
/channels/{채널 id}형식으로 되어 있고, 친구 목록이 뜨는 페이지는 조금 특별하게/channels/@me로 되어 있어요. 경로명 짓기가 번거롭다면 그대로 따라가는 것도 괜찮다고 생각해요
src/frontend/src/pages/FriendsPage/components/CreateGuildModal/index.tsx
Outdated
Show resolved
Hide resolved
| <S.DiscordIcon size={32} /> | ||
| </S.DMButton> | ||
| {/* 서버 리스트 추가 예정 */} | ||
| <S.AddServerButton onClick={() => setCurrentModal('initial')}> |
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.
리팩토링 제안
길드 생성 버튼을 별도의 컴포넌트로 분리해서 눌렀을 때의 동작(모달에서 여러 step 이동)을 따로 관리해도 괜찮을 것 같아요.
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.
현재 퍼널 구조는 CreateGuildModalContent 컴포넌트에서 step들을 관리하는 구조로 만들었어요. 길드 생성 버튼 클릭시 뜨는 모달의 내부를 step별로 보여주는 컴포넌트라고 생각해주시면 될 것 같아요!
피드백을 고려해서 GuildList 컴포넌트에서 해당 버튼을 별도의 컴포넌트로 아래처럼 분리해봤었어요
// GuildList.tsx
const GuildList = () => {
return (
<S.GuildList>
<S.DMButton>
<S.DiscordIcon size={32} />
</S.DMButton>
{/* 서버 리스트 추가 예정 */}
<AddGuildButton />
<S.SearchCommunityButton>
<S.CompassIcon size={36} />
</S.SearchCommunityButton>
</S.GuildList>
);
};
// AddGuildButton.tsx
const AddGuildButton = () => {
const { openModal } = useModalStore();
const handleChangeModal = () => {
openModal('basic', <CreateGuildModalContent />);
};
return (
<S.AddGuildButton onClick={handleChangeModal}>
<S.PlusIcon size={24} />
</S.AddGuildButton>
);
};분리하고 보니, GuildList 컴포넌트에서 AddGuildButton까지 들어와야 CreateGuildModalContent 컴포넌트가 모달에 뜸을 확인할 수 있어서 별도로 분리하지 않고 기존대로 유지하는 방식은 어떨까 생각중인데 의견이 궁금해요!
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.
AddGuildButton 분리는 GuildList 컴포넌트 안에서 step을 관리한다고 생각하고 말씀드렸던 거라, step을 관리하는 별도의 컴포넌트를 분리하셨다면 AddGuildButton까지 분리할 필요는 없을 것 같아요!
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.
헉 답변이 달리기 전에 푸쉬를 해버려서,,ㅎ 다시 커밋찍고 반영해놨습니다! 👍🏻
c5044aa to
24f1b57
Compare
피드백 반영 및 오류 개선
2025-02-14.5.55.14.mov폴더 구조 등 컨벤션이 다소 어색한 부분이 있다면 말씀해주세요! |
chysis
left a comment
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/frontend/src/types/index.ts
Outdated
| day: string; | ||
| } | ||
|
|
||
| export const PopupTypes = { |
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.
팝업은 새 창을 표시하는 방법이라 모달, 바텀시트를 통칭하는 용어로는 적절하지 않다고 생각해요. 바텀시트를 모달의 일종으로 보고, 기존과 같이 ModalTypes로 가도 괜찮을 것 같은데 어떻게 생각하시나요? 😄
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/frontend/src/hooks/useFunnel.tsx
Outdated
| Step, | ||
| step, |
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.
두 변수가 대소문자 하나 차이라 그런지 어떤 것을 의미하는지 헷갈리네요😅 현재 스텝을 나타내는 상태를 currentStep 등으로 수정하는 것은 어떨까요?
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.
말씀처럼 currentStep이 코드를 볼 때 훨씬 가독성이 좋을 것 같아요! 감사합니다 👍🏻
chysis
left a comment
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.
수정 사항 모두 확인했습니다!!
이슈번호
요약(개요)
작업 내용
(gif가 조금 느리네요 ,,,!)
집중해서 리뷰해야 하는 부분
기타 전달 사항 및 참고 자료(선택)
/friends에서 확인 가능하세요!