[FEAT/#20] 채팅 바텀 스낵바(pop_up) 컴포넌트 개발 #21
Conversation
|
Thanks for the contribution! Please review the labels and make any necessary changes. |
Summary of ChangesHello @chunjaemin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 채팅 인터페이스를 위한 재사용 가능한 하단 스낵바 컴포넌트를 도입합니다. 이 컴포넌트는 범용적인 페이드 및 슬라이드 애니메이션 훅을 활용하여 다른 UI 요소에도 적용할 수 있도록 설계되었습니다. 특히 키보드 상호작용을 고려하여 동적인 레이아웃에 원활하게 통합될 수 있도록 스택 기반의 배치 방식을 채택했습니다. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a new BottomSnackbar component with fade and slide animations, along with a useFadeSlideVisibility hook to manage its visibility and animation. The implementation looks clean and well-structured, providing a reusable solution for snackbar-like UI elements. The README.md file is comprehensive and provides clear usage instructions and prop documentation. The useMemo hook in useFadeSlideVisibility is correctly used to memoize animatedStyle, preventing unnecessary re-renders. Overall, the changes enhance the UI capabilities with a well-designed and animated component.
| const animatedStyle = useMemo(() => { | ||
| const translateY = progress.interpolate({ | ||
| inputRange: [0, 1], | ||
| outputRange: [translateYFrom, 0], | ||
| }); | ||
|
|
||
| return { | ||
| opacity: progress, | ||
| transform: [{ translateY }], | ||
| } as const; | ||
| }, [progress, translateYFrom]); |
There was a problem hiding this comment.
The useMemo hook here is correctly used to memoize the animatedStyle object. This is a good practice as it prevents unnecessary re-creations of the style object on every render, which could lead to performance issues, especially with complex animations or frequent re-renders of the parent component. This aligns with the general rule of avoiding useMemo when it only returns a prop without any complex computation, as here it does involve computation (interpolation) and depends on progress and translateYFrom.
References
- Avoid using
useMemowhen it only returns a prop without any complex computation, as it offers no performance benefit and adds unnecessary complexity.
There was a problem hiding this comment.
useMemo로 애니메이션 객체 재생성을 막은 것이라 이대로 유지하도록 하겠습니다
ahcgnoej
left a comment
ahcgnoej
left a comment
There was a problem hiding this comment.
구현하시느라 고생하셨습니다!! 🙇🏻
리뷰 한 번 확인 부탁드립니다!
| if (!finished) return; | ||
| if (!visible) setShouldRender(false); | ||
| }); | ||
| }, [progress, visible, duration]); |
There was a problem hiding this comment.
의존성 배열에 progress가 안 들어가도 정상동작하나요?
useRef는 참조값이 같다고 알고있어서 의존성배열에 들어갈 필요가 있나 싶습니다! 확인부탁드립니당
There was a problem hiding this comment.
피드백 감사합니다! 말씀하신 대로 useRef는 참조값이 변하지 않아 동작상 문제는 없지만, 현재 프로젝트에 적용된 Biome 규칙이 useEffect 내 참조된 변수를 의존성 배열에 포함하도록 권장하고 있네요.
불필요한 변수를 제거해 가독성을 높이는 것도 좋지만, 규칙을 준수함으로써 혹시 모를 누락으로 인한 버그를 예방하려는 의도인 것 같습니다. 가독성과 안전성 사이에서 충분히 논의해 볼 만한 주제인 것 같아요!
당장은 린트 규칙에 따라 이 형태를 유지하되, 나중에 팀 차원에서 의존성 배열 관리 기준을 한번 논의해 봐도 좋을 것 같습니다. 몰랐던 내용이었는데 피드백 해주신 덕분에 저도 깊게 고민해 볼 수 있었네요!
| style={shadows.neutral} | ||
| > | ||
| <Text className="text-lg font-bold text-content-primary">{title}</Text> | ||
| {!!subtitle && ( |
There was a problem hiding this comment.
저도 궁금해서 확인해 보니, !! 연산자는 값을 확실하게 불리언 타입으로 강제 변환하기 위한 방어 코드였네요!
{count && } 같은 코드에서 count가 0일 때 화면에 0이 그대로 출력되는 버그를 막아주는 용도라고 합니다. 하지만 지금 subtitle은 문자열 타입이라 0이 출력될 이슈가 없어서 굳이 안 써도 될 것 같아요. 코드 가독성을 위해 시간 될 때 !!는 정리해 두도록 하겠습니다!"
| if (!finished) return; | ||
| if (!visible) setShouldRender(false); | ||
| }); | ||
| }, [progress, visible, duration]); |
There was a problem hiding this comment.
저도 은혜님처럼 progress 의존성이 불필요하지 않나 생각을 했었는데 현재 저희 프로젝트의 biome 규칙이 의존성 배열에 삽입하도록 추천하고 있군요..!
의존성 배열 관리 기준은 한 번쯤 팀 내에서 논의해 보면 좋을 것 같아요.
useRef처럼 안정적인 참조는 린트 suppress 주석(// biome-ignore)으로 명시적으로 예외 처리하는 방법도 옵션이 될 수 있을 것 같아서요.
이외에는 너무 잘 짜주신 코드같아요.
고생하셨습니다.
chunjaemin
changed the title
3 participants
요약
스낵바 말고도 팝업창과 같은 다른 UI에도 쓸 수 있으니 애니메이션 필요하신 분들은 가져다가 쓰셔도 좋을 것 같아요
참고사항
스낵바는 absolute로 Floating 하지 않고, stack방식으로 돔 요소 위에 쌓이도록 구현했습니다.
키보드 유무에 따라 스낵바의 위치도 같이 움직여야 한다는 점을 고려했을 때 absolute 보다 stack 방식이 동적인 움직임을 더 간편하게 조작할 수 있을 것 같아서 stack방식을 채용했습니다
바텀 스낵바는 widgets에 구현했습니다.
스낵바 visible 상태관리는 props로 받아서 on/off 되도록 최대한 간단하게 구현해놓았습니다.
상태관리를 할 수 있는 방법이 여러가지라 이부분은 은혜님이 사용하시기 편한 방법으로 바꿔서 사용하시면 될 것 같아요 당장에는 가장 직관적으로 사용할 수 있도록 했습니다!
🔗 관련 이슈
💬 리뷰어에게