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

fix: BadgeProps 타입이 HTMLAttribute 확장하도록 수정 #22

Merged
merged 1 commit into from
Nov 19, 2023

Conversation

nijuy
Copy link
Collaborator

@nijuy nijuy commented Nov 16, 2023

1️⃣ 어떤 작업을 했나요? (Summary)

  • BadgePropsReact.HTMLAttributes<HTMLDivElement>를 확장하도록 수정했어요 왜 안했을가요
  • StyledBadge(div)에 나머지 props를 넣어줬어요

4️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?

- 그리고 StyledBadge에 나머지 props를 넣어줬어요
@@ -3,13 +3,14 @@ import { IconContext } from '@/style';
import { StyledBadge } from './Badge.style';
import { BadgeProps } from './Badge.type';

const Badge = ({ color = 'monoItemBG', children = 'Badge', leftIcon }: BadgeProps) => {
const Badge = ({ color = 'monoItemBG', children = 'Badge', leftIcon, ...props }: BadgeProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

혹시 Badge에서 forwardRef를 사용하지 않는 이유가 있나용??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Badge는 사용자랑 상호작용하지 않고, 상태가 1개인 단순 정보 표기용 컴포넌트임
    ➡ 부모 컴포넌트에서 Badge의 상태나 값 같은 내부 요소에 접근할 일이 없을거라고 생각함

  2. YDS-React-old에서도 사용하지 않았음

위와 같은 이유로,, forwardRef를 사용하지 않았습니다!
혹시 사용해야 한다고 생각해서 코멘트 준거였나영?! (╯▽╰ )

Copy link
Member

Choose a reason for hiding this comment

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

아뇨! 달아주신 코멘트 내용이 궁금해서 여쭤봤숩니당 ㅎㅅㅎ

@nijuy nijuy merged commit e969e92 into develop Nov 19, 2023
@nijuy nijuy deleted the feat/badge branch February 7, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants