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

[FE] Dropdown 관련 컴포넌트들 구현 #16

Merged
merged 13 commits into from
Jul 26, 2023
Merged

Conversation

Kakamotobi
Copy link
Member

@Kakamotobi Kakamotobi commented Jul 25, 2023

Issues

What is this PR? 👓

  • Dropdown 관련 컴포넌트들 구현.

Key changes 🔑

  • DropdownIndicator, DropdownPanel, DropdownItem 컴포넌트 구현.
  • designSystem 수정.

To reviewers 👋

  • 흐름(state, props 전달, etc.)을 봐주세요.
  • Types 정의도 확인해주세요.
  • designSystem.ts에 icon용 filter랑 border 추가했는데 확인해주세요.

- DropdownItemType의 종류(`withImg`, `withColor`, `onlyContent`)에 따라 내부 구성 렌더.
- `DropdownNameKOR` enum을 사용하여 영한 번역.
  - 로직은 영어로하고, 렌더할 때 한글로 번역.
- `isOpen`에 따라 `<DropdownPanel />` 렌더.
- 사용 컴포넌트로부터 `dropdownName` 및 `dropdownList`를 props로 전달 받음.
@Kakamotobi Kakamotobi added this to the [FE] Sprint #01 milestone Jul 25, 2023
@Kakamotobi Kakamotobi requested a review from youzysu July 25, 2023 09:21
@Kakamotobi Kakamotobi self-assigned this Jul 25, 2023
@Kakamotobi Kakamotobi closed this Jul 25, 2023
- `DropdownPanel`이 사용되는 상황은 두가지(필터 또는 이슈 생성시 선택)가 있음.
  - 그에 맞게 `panelType`을 `filter`와 `select`로 구분하여 렌더.
@Kakamotobi Kakamotobi reopened this Jul 25, 2023
@Kakamotobi Kakamotobi closed this Jul 25, 2023
- Dropdown 관련 types 정의에서 `type` -> `variant`으로 변경.
- Design system에 `border` 및 SVG icon용 `filter` 추가.
@Kakamotobi Kakamotobi reopened this Jul 25, 2023
Copy link
Member

@youzysu youzysu left a comment

Choose a reason for hiding this comment

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

수고하셨습니다아 👍 (빠르고 정확한 코드에 감탄..)

Dropdown

  • Dropdown 종류가 한가지 더 있는데, 나중에 작업할 때 DropdownPanel에 추가해도 좋고 이번에 추가해도 좋을 것 같아요!

image

designSystem iconFilter & border

designSystem.ts에 icon용 filter랑 border 추가했는데 확인해주세요.

  • 혹시 iconFilter는 어떤 경우 어떤 icon의 filter인가요? 모든 icon이 같은 filter가 적용되지 않는 것 같아서요!
  • border는 저도 PR에 포함해두었는데, 기존처럼 light & dark mode 공통으로 사용하도록 두고 color는 어떤 컴포넌트인지에 따라 동적으로 부여하도록 color 속성은 빼서 사용하면 어떨까요?
    image

Comment on lines +6 to +59
export default function DropdownItem({ item }: { item: DropdownItemType }) {
const generateItem = (item: DropdownItemType) => {
switch (item.variant) {
case "withImg":
return (
<Label htmlFor={item.content}>
<Avatar
src={item.imgSrc}
alt={`${item.variant}: ${item.content}`}
/>
<Content>{item.content}</Content>
<input
className="radio-input"
type="radio"
name={item.name}
id={item.content}
/>
<img className="radio-img" src={checkOffCircle} alt="" />
</Label>
);
case "withColor":
return (
<Label htmlFor={item.content}>
<ColorSwatch $colorFill={item.colorFill} />
<Content>{item.content}</Content>
<input
className="radio-input"
type="radio"
name={item.name}
id={item.content}
/>
<img className="radio-img" src={checkOffCircle} alt="" />
</Label>
);
case "plain":
return (
<Label htmlFor={item.content}>
<Content>{item.content}</Content>
<input
className="radio-input"
type="radio"
name={item.name}
id={item.content}
/>
<img className="radio-img" src={checkOffCircle} alt="" />
</Label>
);
default:
throw Error("Invalid dropdown item variant");
}
};

return <StyledDropdownItem>{generateItem(item)}</StyledDropdownItem>;
}
Copy link
Member

Choose a reason for hiding this comment

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

Label 하위 컴포넌트에 <Content>부터 <img> 까지 중복인 것 같아서 하위 컴포넌트로 빼도 좋을 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

마땅한 하위 컴포넌트 이름이 안떠올라서 넘겼는데 한번 빼볼게요 👍

@Kakamotobi
Copy link
Member Author

Kakamotobi commented Jul 25, 2023

캡쳐까지 첨부하셔서 설명해주시니 이해가 빠르네요. 감사합니다 🙏

  • 위에 언급해주신 놓친 Dropdown 종류는 이번 PR에 반영하는게 좋을 것 같아요. 추가하겠습니다. 👍
  • designSystem 부분은 조이랑 겹칠 것 같아서 일단 임의로 그렇게 했어요. 조이 말씀해주신대로 하면 될거 같아요. 근데 그렇게 되면 헷갈려서 colors객체로 neutral, brand, danger, palette을 래핑해주는게 좋을 것 같아요.

@youzysu youzysu merged commit 7f9bee3 into fe-w1 Jul 26, 2023
@youzysu youzysu deleted the fe/design/#11-dropdown branch July 26, 2023 01:35
youzysu pushed a commit that referenced this pull request Jul 31, 2023
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.

2 participants