-
Notifications
You must be signed in to change notification settings - Fork 5
[FE] feat: 채널 생성 구현 #73
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.
카테고리, 채널 생성 구현하느라 고생하셨어요! 주로 스타일 관련해서 사소한 코멘트 남겼는데, 당장은 반영하지 않아도 되는 것들이긴 합니다...
버그가 있는데, 그 부분은 지금 수정이 필요할 것 같아요!
| const handleChannelNameChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newName = event.target.value; | ||
|
|
||
| if (newName.length !== 0) setChannelName(newName); | ||
| }; |
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.
버그 제보 감사합니다!! 생각치 못했던 부분이었네요 🫠 버튼 활성화 여부도 함께 포함해서 수정해둘게요!
| background-color: ${({ theme, $isSelectedType }) => | ||
| $isSelectedType ? theme.colors.dark[350] : theme.colors.dark[600]}; | ||
|
|
||
| svg { | ||
| color: ${({ theme }) => theme.colors.dark[400]}; | ||
| } | ||
|
|
||
| span { | ||
| color: ${({ theme }) => theme.colors.dark[400]}; | ||
| } | ||
|
|
||
| &:hover { | ||
| cursor: pointer; | ||
| background-color: ${({ theme }) => theme.colors.dark[350]}; | ||
| } |
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.
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가 더 예뻐졌네요!! 👍
| const handleCategoryNameChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newName = event.target.value; | ||
|
|
||
| if (newName.length !== 0) setCategoryName(newName); | ||
| }; |
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.
이 부분도 categoryName이 완전히 지워지지 않는 버그를 발견했습니다!
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.
channel이랑 함께 반영해둘게요 :)
| export const CategoryName = styled.div` | ||
| cursor: pointer; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| width: 100%; | ||
|
|
||
| svg { | ||
| color: ${({ theme }) => theme.colors.dark[300]}; | ||
| } | ||
| `; |
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.
매의 눈 👍🏻 반영해둘게용!
| <S.Channels key={channel.channelId}> | ||
| <BodyRegularText>{channel.name}</BodyRegularText> | ||
| </S.Channels> |
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.
채널 타입에 따라 channel.name 앞에 맞는 아이콘을 넣으면 좋겠어요! 당장 반영하지 않아도 괜찮습니다.
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.
| export const Channels = styled.div` | ||
| display: flex; | ||
| `; |
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.
사소하지만, channel의 경우 padding-left로 indent를 주면 가독성이 더 좋아질 것 같아요!
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.
사소함이 굉장히 큰 변화네요 ✨ 덕분에 가독성이 훨씬 나아진 것 같아요!
| isOn: boolean; | ||
| onToggle: () => void; | ||
| } | ||
| const Toggle = ({ isOn, onToggle }: ToggleProps) => { |
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.
애니메이션 동작까지 좋아요 👍
컴포넌트 이름을 ToggleButton으로 두는 것은 어떤가요?
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.
Toggle, ToggleSwitch, ToggleButton 등 다양하게 불리는 것 같은데 전 Toggle이 깔끔해보여서 요건 그대로 유지해둘게요! 의견 감사합니당 :)
| export const Channels = styled.div` | ||
| display: flex; | ||
| `; |
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.
cursor: pointer도 주면 좋을 것 같습니다 :)
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.
:완완:
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.
코멘트 적극적으로 반영해주셔서 감사해요 ㅎㅎ
한 가지 버그가 더 발견돼서 다시 제보드립니다!! 확인 부탁드려요
| export const CreateButton = styled.button<{ $disabled: boolean }>` | ||
| width: 11rem; | ||
| height: 4rem; | ||
| border-radius: 0.8rem; | ||
|
|
||
| color: ${({ theme }) => theme.colors.white}; | ||
|
|
||
| background-color: ${({ theme, $disabled }) => ($disabled ? theme.colors.dark[400] : theme.colors.blue)}; | ||
| `; |
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.
뜨헉 버튼 속성을 위한 disabled 추가할게요! 😅 추가로 cursor도 not-allowed 처리해둘게요 ㅎㅎ
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, $disabled를 모두 사용해도 스타일 prop으로 구분돼서 크게 어색하진 않네요!
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.
수정 사항 확인했어요! 고생하셨습니다 😊






이슈번호
요약(개요)
길드의 카테고리 및 채널 정보 조회, 채널 생성 기능을 구현합니다
작업 내용
집중해서 리뷰해야 하는 부분
기타 전달 사항 및 참고 자료(선택)
공통 컴포넌트로 쓰일
toggle컴포넌트를 만들어놨어요! 해당 컴포넌트에 예시 및 prop 설명을 함께 뒀어요!2025-02-23.5.56.40.mov