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

[Feat] showcase 페이지 category chip 구현 #60

Merged
merged 13 commits into from
Jul 9, 2024

Conversation

Bowoon1216
Copy link
Contributor

@Bowoon1216 Bowoon1216 commented Jul 9, 2024

해당 이슈 번호

closed #53


체크리스트

  • 🔀 PR 제목의 형식을 잘 작성했나요? e.g. [feat] PR을 등록한다.
  • 💯 테스트는 잘 통과했나요?
  • 🏗️ 빌드는 성공했나요?
  • 🧹 불필요한 코드는 제거했나요?
  • ✅ 컨벤션을 지켰나요?
  • 💭 이슈는 등록했나요?
  • 🏷️ 라벨은 등록했나요?
  • 💻 git rebase를 사용했나요?
  • 🙇‍♂️ 리뷰어를 지정했나요?

📌 알립니다..

image

  • 이 컴포넌트를 여러개 사용할 때, 하나를 클릭했을 때 다른 것은 클릭이 해제되어야하는 로직을 위해
    image
  • clickedChip을 인자로 받고, 이 clickedChip는 해당 컴포넌트의 children과 같을 수 있습니다.
  • 여기서 같을 때가 해당 컴포넌트를 클릭한 때라고 생각하시면 됩니다

📌스크린샷 (선택)

image
//clickedChip 문자열이 children이랑 다를 때

image
//clickedChip 문자열이 children이랑 같을 때

Copy link
Member

@namdaeun namdaeun 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 19 to 20
'&:hover': { backgroundColor: theme.colors.blue_100 },
});
Copy link
Member

Choose a reason for hiding this comment

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

요기 가독성 좋게

    '&:hover': { 
           backgroundColor: theme.colors.blue_100 
    },

요런식으로 줄바꿈해서 작성해주셔용!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

인정합니다! 감샤합니다 👍


interface CategoryChipProps extends Omit<ComponentPropsWithRef<'button'>, 'onClick'> {
onClick?: (id: string) => void;
clickedChip?: string;
Copy link
Member

Choose a reason for hiding this comment

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

클릭된 여부에 따라 string값보다 boolean 타입으로 isSelected와 같은 값으로 전달해주는 건 어떤가요?
그리고 그 불리언 값에 따라 스타일 변화하는 식으로 작성해주면 좋을 것 같아요!

Copy link
Contributor

Choose a reason for hiding this comment

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

clickedChip은 현재 어떠한 chip이 클릭되어있는지 알기 위한 prop인거 같아서 현재 코드에서는 boolean 값으로 받는게 맞는거 같습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네! 문자열로 받은 이유는 이 컴포넌트가 ref를 반환하지 않아서 어떤 버튼을 클릭했는지 알지 못합니다
그래서 다른 버튼을 눌렀을 때 클릭되어 있던 버튼을 언클릭하는 로직을 각 컴포넌트 안에 넣은 것 입니다
(+이 로직을 밖으로 뺄 경우 사용할 때 복잡해질 것 같아서)

Comment on lines 14 to 18
if (clickedChip === children?.toString()) {
setIsClicked(true);
} else {
setIsClicked(false);
}
Copy link
Member

Choose a reason for hiding this comment

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

경우의 수가 하나이니까 삼항연산자로 구현해줘도 될 것 같습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

인정합니다! 좋은지적 감사합니다 😄

Copy link
Contributor

@rtttr1 rtttr1 left a comment

Choose a reason for hiding this comment

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

LGTM~~


interface CategoryChipProps extends Omit<ComponentPropsWithRef<'button'>, 'onClick'> {
onClick?: (id: string) => void;
clickedChip?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

clickedChip은 현재 어떠한 chip이 클릭되어있는지 알기 위한 prop인거 같아서 현재 코드에서는 boolean 값으로 받는게 맞는거 같습니다!

Copy link
Contributor

@wuzoo wuzoo left a comment

Choose a reason for hiding this comment

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

너무 잘짜신 것 같아요 ㅎㅎ chip을 어떻게 관리할지에 대한 고민이 보입니다.

Comment on lines 13 to 15
useEffect(() => {
clickedChip === children?.toString() ? setIsClicked(true) : setIsClicked(false);
}, [clickedChip, children]);
Copy link
Contributor

Choose a reason for hiding this comment

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

위에서 다른 팀원들이 말한 것처럼, chip 컴포넌트는 isSelected라는 불리언 값을 전달받고 그에 따른 UI를 표시해주는 것이 더 좋을 것 같아요 !

부모 컴포넌트에서 selectedChip을 관리하고, prop으로 selectedChip === currentChip을 넘겨주는 정도로 구현하면 될 것 같네요 !

또한 안에서 handleClick을 만들어주지 않아도 될 것 같습니다. chip은 그냥 click 핸들러를 연결만 해주고, 위에서 해당 chip을 클릭했을 시의 동작을 핸들러로 생성하여 넘겨주면 될 것 같네요 !!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵! 인정합니다! 👍 수정했습니다 ~

Copy link
Contributor

@cindy-chaewon cindy-chaewon left a comment

Choose a reason for hiding this comment

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

LGTM 입니당!! 고생하셨어요!

@Bowoon1216 Bowoon1216 merged commit ed0fa90 into develop Jul 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

showcase 페이지 CategoryChip 컴포넌트
5 participants