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(system): Dropdown 컴포넌트 구현 #26

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Conversation

qkrdmstlr3
Copy link
Member

@qkrdmstlr3 qkrdmstlr3 commented Aug 20, 2024

3. 관련 스크린샷을 첨부해주세요.

2024-08-21.1.29.50.mov
스크린샷 2024-08-21 오전 1 33 12

4. 완료 사항

  • Dropdown 컴포넌트 구현
<Dropdown>
  <Dropdown.Trigger>
    <Dropdown.TriggerArrow />
  </Dropdown.Trigger>
  <Dropdown.Content>
    <Dropdown.CheckedItem></Dropdown.CheckedItem>
    <Dropdown.CheckedItem></Dropdown.CheckedItem>
    <Dropdown.CheckedItem></Dropdown.CheckedItem>
  </Dropdown.Content>
</Dropdown>

5. 추가 사항 / 코드 리뷰 받고 싶은 부분

  • 공통적으로 사용되는 곳이 많아 구현합니다.
    • 기존에 shadcn의 UI와 스펙이 우리와 맞지 않는 부분이 많아 필요한 부분만 간단히 재구현합니다.
  • Anatomy를 기반으로 styles과 logic레이어를 분리합니다.
    • context를 사용해서 각 컴포넌트에서 style과 logic을 받아 사용합니다.
    • 레이어를 분리함으로써 드롭다운 커스텀의 니즈가 있다면 useDropdownStyles와 useDropdownLogic을 사용하면 원하는 부분만 커스텀하여 구현할 수 있습니다.

Copy link

netlify bot commented Aug 20, 2024

Deploy Preview for bbogak failed.

Name Link
🔨 Latest commit d08e706
🔍 Latest deploy log https://app.netlify.com/sites/bbogak/deploys/66c5f2b6a7373c0008db80db

@qkrdmstlr3 qkrdmstlr3 force-pushed the feat/system/dropdown branch from bf9b3c1 to 5ff2f3e Compare August 20, 2024 16:58
Copy link
Collaborator

@woo-jk woo-jk left a comment

Choose a reason for hiding this comment

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

혹시 컴포넌트의 사용법은 어떻게 될까요? 공통 컴포넌트 제작시에 컴포넌트 사용법도 함께 공유해주시면 좋을것 같습니다! 코드 리뷰하기에도 더 수월할 것 같아요~!

Copy link
Member

@Collection50 Collection50 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 !

몇가지 코멘트 남겨드립니다 👍

src/system/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
Comment on lines 14 to 28
export const CheckboxItem = forwardRef<HTMLButtonElement, Props>(function CheckboxItem(
{ checked, disabled, children, ...restProps },
ref,
) {
const { styles, logics } = useDropdownContext();

return (
<button ref={ref} disabled={disabled} {...mergeProps(styles['checkbox-item'], logics['checkbox-item'], restProps)}>
{children}
<If condition={checked}>
<Icon name="check" color={color.neutral30} size={20} />
</If>
</button>
);
});
Copy link
Member

Choose a reason for hiding this comment

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

Dropdown에 Checkbox가 필요한 이유에 대해 고민한 시간이 존재했습니다.
이후 영상을 보고 어떤 컴포넌트인지 파악했어요
CheckedItem이라는 이름은 어떨까요??

코멘트 수준으로 남겨드립니다 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

좋습니다!

separator: {},
'trigger-arrow': {},
'checkbox-item': {
onClick: () => onOpenChange(false),
Copy link
Member

Choose a reason for hiding this comment

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

chain을 사용해서 onClick을 체이닝하는 것이 좋다고 생각합니다 !
임의로 추가되는 onClick에 대해서도 대응하면 좋을 것 같아요 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

컴포넌트 단계에서 mergeProps로 합쳐주고 있어요!

Copy link
Member

@eunbeann eunbeann left a comment

Choose a reason for hiding this comment

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

👀 수고하셨습니다!!

Copy link
Member

Choose a reason for hiding this comment

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

이건 나눠진 리스트에 divider 쓸 때 필요한거죠??
image

Copy link
Member Author

Choose a reason for hiding this comment

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

네!

@qkrdmstlr3
Copy link
Member Author

PR예제 본문에 첨부했습니다!

@qkrdmstlr3 qkrdmstlr3 merged commit 79e6cd2 into main Aug 21, 2024
0 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants