-
Notifications
You must be signed in to change notification settings - Fork 52
Re-implement Tabs component using Radix #1061
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
Conversation
🦋 Changeset detectedLatest commit: 5a8520a The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Chromatic Report🚀 Congratulations! Your build was successful! |
Codecov ReportBase: 72.37% // Head: 72.61% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## next-v1 #1061 +/- ##
===========================================
+ Coverage 72.37% 72.61% +0.23%
===========================================
Files 223 237 +14
Lines 3070 3137 +67
Branches 844 853 +9
===========================================
+ Hits 2222 2278 +56
- Misses 722 728 +6
- Partials 126 131 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
1f458bc to
c7afc43
Compare
d363d57 to
297dccc
Compare
sungik-choi
left a comment
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.
컴포넌트 전반적으로 default export를 named export로 변경 부탁드립니다! (#931 참고)
4bfbfd3 to
b288eda
Compare
d3b27f8 to
55775d9
Compare
Dogdriip
left a comment
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.
🚀
sungik-choi
left a comment
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.
👍 마이너한 코멘트들 남겼습니다.
- #1086 머지 이후에 배포 후 데스크 어플리케이션에 적용할 예정이라, 이후에 머지해주시면 좋을 거 같습니다.
- 마이그레이션도 함께 챙겨주시고, 마무리 이후 레거시 컴포넌트도 정리해주시면 👍
| justify-content: space-between; | ||
| width: 100%; | ||
| height: ${props => props.size}px; | ||
| height: ${({ size }) => heightBy(size)}px; |
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.
케이스가 3개만 정해져있어서 유의미하진 않지만, className 재생성을 막기 위해 CSS Variable로 스타일링할 수도 있습니다.
| border-radius: 1.5px; | ||
| ${({ foundation }) => foundation?.transition?.getTransitionsCSS(['transform'])}; | ||
| will-change: transform; |
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.
Warning: will-change is intended to be used as a last resort, in order to try to deal with existing performance problems. It should not be used to anticipate performance problems. (#)
정확한 측정은 필요하겠지만, 성능상 문제가 없을거로 예상되는데 굳이 필요한가? 라는 생각입니다.
…est code for tabaction(wip)
…ing on href is given or not
…ode associated with it
76ec437 to
5a8520a
Compare
Self Checklist
CODEOWNERSfile.Related Issue
Tabs,TabItemcomponent usingTabsin Radix UI #982Summary
Details
Tabs컴포넌트의 사용법이 달라지게 됩니다. Radix 에서 value 기반으로Tabs.Trigger와Tabs.Content를 매칭시키다보니, 기존에Tabs에 number type 의index를 주던 것을 string type의value를 주는 것으로 변경하였습니다. 이렇게 하면서TabItem에 다소 불필요하게optionKeyprop 을 열어주던 것을 제거하고, handler 에서는value만 인자로 가지도록 했습니다. (아래 코드 참고)As-is
To-be
controlled 와 uncontrolled 두 형태로 모두 사용가능하게 되었습니다. controlled 형태로 사용하려면
value와onValueChange에 state와 setter를 넣고, uncontrolled 형태로 사용하려면defaultValue에 값을 넣으면 됩니다.test code 를 개선하였습니다. testing library 의 query priority 에 따르면 element 의 role과 accessible name 에 의해 query 하는 것이 권장되기 때문에 그러한 query 방식을 사용하여 test code 를 작성하였습니다.
keyboard navigation 이 가능해집니다.
TabItem을 클릭하고 화살표로 좌우 이동하게 되면 다른TabItem과TabContent를 활성화 시킬수 있고, 탭 키로 포커싱되는 element 를 변경할 수 있습니다. 다만 데스크 코드에서는 탭을 클릭했을 때 다른 url 로 이동하는 식으로 작성된 부분이 있는데, 이 경우는TabContent에 해당하는 컴포넌트가 Routes 에 모여져 있어서 탭 키에 의한TabContent포커싱은 어려울 것으로 생각됩니다.피그마 시안에서 베지어 버튼으로 구성된
TabItem을 베지어 버튼으로 구현했습니다.TabAction은 베지어 버튼과 미묘하게 다른 부분이 있어서Text와LinkIcon을 활용한 기존 구현을 그대로 두었습니다.하나 아쉬운 부분이 있는데, 다른
TabItem이 활성화될 때 underline 이 올라오는 애니메이션을 그대로 옮기지 못했습니다. 기존에는TabItem바로 아래에 overflow:hidden 속성을 가지는Wrapper를 두고 활성화 되었을 때 ::after 로 구현된 underline 이 위로 올라오는 애니메이션이 있었는데, 새로 구현된TabItem같은 경우Tabs.Trigger의 접근성 관련 속성을 베지어 버튼으로 바로 전달해주기 위해Wrapper를 주기가 곤란했습니다. 그런데 이렇게 하면 버튼 밖에 있는 underline 을 보여줘야 해서 overflow:hidden 을 주지 못하게 되서 애니메이션 역시 보여줄 수 없게 됩니다. 혹시 workaround 가 떠오르는 분이 있다면 의견 부탁드립니다.ToDos
Breaking change or not (Yes/No)
TabItem을 선택하게 되고, 선택되는 탭에 매칭되는 content 는TabContent으로 래핑해야 합니다.References