Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] BottomSheet 컴포넌트, 스토리북 #41
[FEAT] BottomSheet 컴포넌트, 스토리북 #41
Changes from 1 commit
5af13cf
3d3cf25
7bf1a90
366e0cb
70ffaf2
eb7d761
beeb54a
a92c437
c1a3f12
c63dfac
2279cf6
631810a
90d4e6a
f273b49
f8f8c1f
390eb14
5bf5e10
489baf2
b2f0822
f3ff5ab
a4d2f8b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
resetTrigger
값이0
인 경우BottomSheet
가close
상태인건가요??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.
resetTrigger가 0인 경우가 BottomSheet의 상태를 나타내는 것은 아니고 사이드메뉴의 즐겨찾기 버튼을 한 번도 누르지 않은 초기 상태를 말합니다
resetTrigger 값이 바뀔 때마다(버튼을 누를 때마다) BottomSheet가 initialHeight(=150)으로 맞춰지며 BottomSheet가 열리게 됩니다
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.
그럼 0보다는 boolean이 더 적합하지 않나요? 숫자형으로 하신 이유가 궁금합니다!
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.
엇 맞아요 그래서 요건 지금 boolean으로 관리하고 있습니다!
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.
🛠️ Refactor suggestion
매직 넘버를 설정 가능한 props로 변경 필요
initialHeight
와minVisibleHeight
가 하드코딩되어 있습니다. 이 값들을 props로 받아 컴포넌트의 유연성을 높이는 것이 좋겠습니다.📝 Committable suggestion
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.
🛠️ Refactor suggestion
백드롭 투명도 전환 로직 개선 필요
현재 백드롭의 투명도 전환이 하드코딩된 값(10)에 의존하고 있습니다. 이는 버그를 유발할 수 있으며, 의도를 파악하기 어렵게 만듭니다.
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.
shadow 값이 혹시 적용이 안돼서 요렇게 하신건가요?!
shadow-[0px_-4px_10px_0px_rgba(0,0,0,0.1)]
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.
넵 기본 tailwind shadow 설정을 적용했을 때는 그림자가 잘 보이지 않아서 커스텀 값 넣었습니다
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.
style
과tailwindCSS
둘 다 사용하신 이유가 있으신가욥??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.
sheetHeight은 드래그에 따라 동적으로 변하는 값이어서 tailwind를 통해 적용하기 어려워, style 속성을 따로 적용했습니다
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.
tailwind
도 동적으로 변경 가능해요~!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.
아하 JIT로 tailwind도 동적으로 적용 가능한 것 확인했습니다!
그렇지만 바텀시트 특성상 드래그에 따라 sheetHeight 값이 자주 변하고, css 속성 중 height 값만 변하기 때문에 동적 클래스로 적용하는 않는 편이 성능이나 유지보수에서 더 나을 것이라고 생각하는데 그대로 두어도 괜찮을까요?
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.
height
만 동적으로 변경하면 되지 않을까요?? 성능이나 유지보수 어디에 문제가 생기는지 궁금합니당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.
문제가 생긴다기 보다..! tailwind에서 매번 새로운 동적 클래스를 생성하는 성능보다 style에서 직접 다루는 것이 더 나을 거라고 생각했습니당
큰 차이가 없다면 통일성을 위해서 tailwind로 바꿀까요?
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.
어짜피 계산된 값이 들어갈텐데, style속성에 넣어도 새로운 값을 업데이트 하는건 똑같지 않나요??
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.
저의 생각은 굳이 style,tailwind를 나눌 필요는 없어 보여요! 유지보수 측면에서도 css가 나눠져 있는 것 보단 하나에서 관리 하는게 좋을 것 같아요. 실제 성능상 유의미한 변화가 있는 지 체크 해보시면 좋을 것 같습니다~!
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.
아하 알겠습니다! 확인해보고 수정해서 올리겠습니당
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.
요 속성들이 다 적용되고 있는게 맞나요??
불필요한 속성이 있다면 지워도 될 것 같아요.
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.
네 이 속성들은 다 적용되고 있습니당
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.
framer 관련 객체들 저는 constants의
motion.ts
에서 관리하는데, 따로 빼도 깔끔할것 같네요!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.
아하 확인했습니다! 수정해서 올리겠습니당
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.
🛠️ Refactor suggestion
접근성 개선이 필요합니다
현재 구현에는 다음과 같은 접근성 관련 개선사항들이 필요합니다:
또한 성능 최적화를 위해 다음 사항들을 고려해보세요:
AnimatePresence
컴포넌트를 별도의 컴포넌트로 분리