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

Drawer 컴포넌트를 구현한다 #29

Merged
merged 4 commits into from
May 21, 2023
Merged

Drawer 컴포넌트를 구현한다 #29

merged 4 commits into from
May 21, 2023

Conversation

liswktjs
Copy link
Contributor

@liswktjs liswktjs commented May 5, 2023

close #19

Drawer 컴포넌트를 구현하였습니다

구현 내용

  • isOpen 값을 boolean으로 받아 drawer의 열림 여부를 보여준다
    • isOpen이 true일 경우 animation을 통해 drawer가 열린다
  • anchor
    • 방향을 정해서 열리게 할 수 있다

궁금한 점

  • width와 height를 유저가 커스텀할 수 있도록 해야하는 것 아닌 가 라는 생각이 듭니당 의견 주세유!

@liswktjs liswktjs added the payments💳 페이먼츠 팀 이슈입니다 label May 5, 2023
@liswktjs liswktjs requested review from soyi47 and yunjin-kim May 5, 2023 12:01
@liswktjs liswktjs self-assigned this May 5, 2023
@intae92 intae92 changed the base branch from release-w18 to release-w19 May 8, 2023 14:13
Copy link
Contributor

@soyi47 soyi47 left a comment

Choose a reason for hiding this comment

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

width와 height를 유저가 커스텀할 수 있도록 해야하는 것 아닌 가 라는 생각이 듭니당 의견 주세유!

사용자가 drawer 내부에 넣는 요소의 width나 height와 동일하게 가지도록 할 수 없으려나요?
직접 drawer 컴포넌트에 어떤 Prop을 주기보다 내부 요소를 자유롭게 스타일링하고 거기에 따라 drawer가 움직이는게 사용하기 편할 것 같아서요!
fit-content 같은 값을 적용할 수 있을지 한번 보면 좋을 것 같슴다.

Comment on lines 16 to 19
anchor: {
defaultValue: 'bottom',
description: 'drawer가 열리는 방향에 따라서 값을 조정할 수 있습니다'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

radio로 보여주는 거 어때요?!

Suggested change
anchor: {
defaultValue: 'bottom',
description: 'drawer가 열리는 방향에 따라서 값을 조정할 수 있습니다'
}
anchor: {
defaultValue: 'bottom',
description: 'drawer가 열리는 방향에 따라서 값을 조정할 수 있습니다',
control: {
type: 'radio'
},
options: ['top', 'bottom', 'left', 'right']
}

Comment on lines +44 to +70
(anchor === 'top' &&
css`
width: 100%;
height: 50%;
border-radius: 0 0 10px 10px;
top: 0;
`) ||
(anchor === 'left' &&
css`
width: 50%;
height: 100%;
border-radius: 0 10px 10px 0;
left: 0;
`) ||
(anchor === 'right' &&
css`
width: 50%;
height: 100%;
border-radius: 0 10px 10px 0;
right: 0;
`) ||
(anchor === 'bottom' &&
css`
width: 100%;
height: 50%;
bottom: 0;
border-radius: 10px 10px 0 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

config로 빼는게 깔끔하지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

config로 뺀다는 것이 어떤 의미일까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

제 pr에서 확인 부탁드립니다.

Comment on lines 29 to 30
width: 0;
height: 0;
Copy link
Contributor

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.

이제와 돌이켜보니 이유가 생각나지 않습니다... 일단 제거하겠습니다 👍

anchor: 'left' | 'right' | 'top' | 'bottom';
}

const Drawer = ({ isOpen, anchor, children }: PropsWithChildren<DrawerProps>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

dimmer에 클릭 이벤트 부착하는건 어떤가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 좋은 생각입니다!

Comment on lines 11 to 15
<S.Dimmed className={isOpen ? 'is-open' : ''}>
<S.Container className={isOpen ? 'is-open' : ''} anchor={anchor}>
{children}
</S.Container>
</S.Dimmed>
Copy link
Contributor

Choose a reason for hiding this comment

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

Container가 Dimmed에 감싸져있을 필요가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Container 컴포넌트 위치를 고정시켜야해서 dimmer로 감싸는 것이 편리하다고 생각하였습니다
추가로 유저가 어떤 위치에 Drawer를 위치 시킬지 변수가 많다고 생각해서 dimmer랑 set로 이동시키는 것이 좋다고 생각했습니다!

Comment on lines 11 to 12
<S.Dimmed className={isOpen ? 'is-open' : ''}>
<S.Container className={isOpen ? 'is-open' : ''} anchor={anchor}>
Copy link
Contributor

Choose a reason for hiding this comment

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

isOpen가 내부에서 필요한가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isOpen을 통해서 Drawer가 열릴 때 드르릭 하는 애니메이션을 구현하기 위해 넣어주었습니다!

@liswktjs liswktjs requested review from soyi47 and yunjin-kim May 14, 2023 12:05
@intae92 intae92 changed the base branch from release-w19 to release-w20 May 14, 2023 18:11
Copy link
Contributor

@soyi47 soyi47 left a comment

Choose a reason for hiding this comment

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

width와 height를 유저가 커스텀할 수 있도록 해야하는 것 아닌 가 라는 생각이 듭니당 의견 주세유!

사용자가 drawer 내부에 넣는 요소의 width나 height와 동일하게 가지도록 할 수 없으려나요? 직접 drawer 컴포넌트에 어떤 Prop을 주기보다 내부 요소를 자유롭게 스타일링하고 거기에 따라 drawer가 움직이는게 사용하기 편할 것 같아서요! fit-content 같은 값을 적용할 수 있을지 한번 보면 좋을 것 같슴다.

요거 반영 되었나요?

@yunjin-kim yunjin-kim merged commit 31e0702 into release-w20 May 21, 2023
@yunjin-kim yunjin-kim deleted the feat/19 branch May 21, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
payments💳 페이먼츠 팀 이슈입니다 release-w20
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants