-
Notifications
You must be signed in to change notification settings - Fork 1
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/drawer: Drawer 컴포넌트 구현 #109
Conversation
- Drawer.Trigger 요소 등에서 재사용 가능한 타입 - ChildProps 네이밍을 통해 single child임을 함의
가독성을 위해 상수명 수정
disclosure 패턴 가이드에 따라 포커스 이동
데스크탑 환경까지 고려해 position default value left로 변경
- 열림 상태를 토글하도록 변경 - e.preventDefault 추가
Portal을 사용하고 있어 Toast 스토리와 동일한 이슈(하나의 root에 전부 렌더링되는 문제)가 있어요. 뒤늦게 발견해서 스토리는 우선 Canvas 탭에서 확인해주시면 감사하겠습니다.🥹 주말 내로 Docs 탭을 수정할 예정이에요.. 어떻게 할 수 있을지 생각하다가 스토리를 위한 sub component를 만드는 것 보다는, Drawer 컴포넌트 스타일 자체가 컨테이너에 의존성을 가지는만큼 props로 컨테이너 요소의 id를 지정할 수 있게 하면 어떨까 싶었어요. 이렇게하면 사용처에서도 더 유연하게 사용할 수 있을 것 같은데 어떤가요? 스토리 관련 수정 계획이에요. 본문에 있던건데 PR이 너무 길어서 .. 댓글로 관심사를 분리해요 ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨어요!!
상위 컨테이너 id를 props로 전달받도록하는 부분은 좋은거 같아요. 사용하는 상황에 맞춰 상위 컨테이너 변경이 필요할 수 있으니까
데스크탑 뷰는.. 사용해보면서 차차 수정해야할거 같아요
useEffect(() => { | ||
const $portal = document.querySelector(`#${containerId}`); | ||
if (!$portal) return; | ||
|
||
$portal.addEventListener('keydown', onKeyDown); | ||
|
||
return () => { | ||
$portal.removeEventListener('keydown', onKeyDown); | ||
}; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 Modal은 background를 따로 띄워서 거기에 클릭이벤트 발생시 닫아 줬는데, Modal도 이런식으로 변경해주는게 좋을까요? 고민되네요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서랍도 dimmer 누르면 닫히고 이건 키보드 접근성 구현한 부분이에요.
모달에도 접근성 구현하신다면 아래 레퍼런스 참고하시면 좋을 것 같아요 ~ 배포는 이대로하고 이슈 추가로 열어도 괜찮을 것 같구요! ㅎㅎ
https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨어요~ 계속 서랍서랍 노래를 부르시던 이유가 있었네요 ㅋㅋ.. 너무 복잡해요
서랍 스타일은 불가피하게 상위 컨테이너에 의존도가 있어요. bottom일 때 width: 100%; left일 때 height: 100%;을 가지도록 했는데 Portal 상위에 컨테이너가 없다보니 bottom인 경우에 이런식으로 스타일링이 되더라고요.
스크롤이 생기는 문제를 말씀하시는게 맞나요? 맞다면 position을 fixed로 하면 어떨까요? 서랍도 결국 Modal처럼 화면 전체를 덮는 성격이라 fixed를 사용해도 될 것 같다는 생각이 들었어요.
하나 궁금한게 있는데, 375px이 모바일의 어떤 표준 규격인가요? 레퍼런스가 있다면 읽어보고 싶어요.
너무 프리패스인 것 같아서 열심히 읽었는데 큰 문제 없어 보여요 고생하셨어요!!
src/components/Drawer/index.tsx
Outdated
background-color: ${black}; | ||
opacity: 20%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모달처럼 dimmed layer를 사용하는 다른 컴포넌트와 맞추면 좋을 것 같아요. mixin이나 그런걸로 정의할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋아요 통일하는게 좋겠어요 ~~
아마 모달이 먼저 머지될 것 같으니 @leesunmin1231 님이 dimmerStyle 스타일 디렉토리에 빼놔주세요 ㅎㅎ
이 코멘트의 결론: 기본값이 화면 전체를 덮는게 맞다고 생각돼서 feat: containerId가 주어지는 경우 Portal style props 전달에서 수정했습니다. 프로토타입 기준으로 모바일부터 생각하다보니 이상한 흐름을 탔던 것 같아요 ..
지금 스토리 Docs 수정하면서 고민하다가 우재님 코멘트 보고 생각한건데 서랍이 항상 화면 전체를 덮도록 하는게 맞을까요? 넘블 트위터 요구사항처럼 반응형을 구현할 때, bottom drawer가 데스크탑 전체 화면을 덮는게 이상해서 첨부한 캡쳐처럼 되는게 맞지 않을까 생각했는데 지금 그게 아닌가.. 고민하고 있어요.. 아래에서 얘기한 의존도가 너비와 dimmer가 차지하는 영역에 관련된 부분이었어요.
이건 레퍼런스 따로 없고 저희 피그마의 규격이에요 ㅎㅎㅋㅋㅋㅋ
|
- 기본 스타일은 fixed로 제공 - 커스텀 컨테이너를 사용하는 경우 Portal, Drawer 스타일링 조정
🤠 개요
Closes Feat: Drawer 컴포넌트 구현 #86
Drawer 컴포넌트를 구현합니다. 감춰진 컨텐츠 영역이 트리거 동작에 슬라이딩 애니메이션과 함께 드러나고, dimmer 영역을 클릭해 닫을 수 있는 컴포넌트입니다.
💫 설명
접근성 라벨과 ID에 사용될 label, 위치를 결정하는 position, 스타일 커스텀을 원하는 경우에 사용할 수 있는 containerId를 props로 가집니다.
Dropdown과 마찬가지의 이유로 Trigger에는 cloneElement를 사용했습니다.
키보드 접근성은 닫힌 상태에서 트리거 요소로 서랍을 열고, 열려있는 상태에서 esc 키로 탈출할 수 있게 했습니다.
서랍이 열리면 포커스가 패널로 이동합니다.
열린 상태에서는 서랍 레이어 외의 요소와 상호작용 할 수 없습니다.
그 외 마이너한 변경들
ChildProps, SetState 유틸리티 타입이 추가되었어요. (이제껏 불편하게 쓰다가 이제와서야..)
PORTAL_ROOT_ID 상수가 여러 컴포넌트를 관리하다보니 prefix로 컴포넌트명이 오는게 읽기 편한 것 같아 수정했어요.
TBD
Modal 컴포넌트와 dimmer 스타일 통일
서랍이 닫히는 경우의 권고사항인 트리거로 focus 이동
까지 구현하려고 했는데, cloneElement를 적용해두어서 triggerRef를 관리하는게 까다로워 고민거리로 남겨놓습니다. (children이 forwardRef가 적용되지 않은 Function Component인 경우가 있어요. 지금 생각하기로는 interactive 요소의 중첩 또는 접근성 가이드 중 하나를 포기해야 할 것 같은데 포커스 이동보다 올바른 시맨틱이 우선이에요.)
bottom drawer 핸들 구현
⭐️ 중요하고 피드백이 필요했지만 해결된 내용
(해결은 #109 (comment) 참고)
⭐️ 중요하고 피드백이 필요해요
서랍 스타일은 불가피하게 상위 컨테이너에 의존도가 있어요. bottom일 때 width: 100%; left일 때 height: 100%;을 가지도록 했는데 Portal 상위에 컨테이너가 없다보니 bottom인 경우에 이런식으로 스타일링이 되더라고요.
Portal 없이 position 속성만 사용할지 잠시 고민했는데 상위 요소 스타일링에 영향을 받는 문제가 해결되는게 아니고, 컨테이너 외의 상위 컴포넌트에 relative가 명시되어 있다든지.. 고려할 점이 더 많아서 기각했어요.
SCSS의
@at-root
문법도 요소별 클래스 네임으로 스타일을 관리하는 CSS-in-JS에서는 사용이 불가능해요.상위 컨테이너 id를 props로 전달받도록하고 관련 내용과 스타일링 필수 속성, 용례를 스토리에 추가했어요. 아래의 댓글과 작업 참고해주세요.
Feat/drawer: Drawer 컴포넌트 구현 #109 (comment)
feat: containerId props 추가 및 스토리에 반영
스토리와 추후 개발에 사용하기 위해 최대 width가 375px로 고정되는
MobileContainer
컴포넌트를 만들었어요.CdsProvider
자체에 반응형 컨테이너를 넣는건 라이브러리 사용 자체에 제약이 생기기 때문에 기각했어요.모달이나 토스트도 포털을 사용하고 있기 때문에 똑같이 적용되는 내용이에요.
데스크탑 뷰에서는 영역을 어떻게 써야하는지 같이 고민해봤어야 하는데 이런 이슈가 있다는걸 너무 늦게 깨달았어요.
지금 방식이 괜찮은지 발생할 수 있는 이슈가 없는지 의견 주시고 !!! 사용하면서도 피드백 해봐요 🤓
📷 스크린샷
2023-04-24.3.26.44.mov