Conversation
![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
This pull request refactors the SearchBar component to enhance reusability and maintainability. Key changes include adding height and fontSize props, implementing a heightClassMap for styling, and improving form state management by using useForm's reset functionality to sync with URL search parameters. Additionally, the SearchBar was moved to a shared components directory, and its usage was updated across the application. A review comment suggests implementing a fontSizeClassMap to maintain consistency with the existing height styling logic.
| fontSize === 'lg' | ||
| ? 'text-lg placeholder:text-lg' | ||
| : 'text-base placeholder:text-base' |
There was a problem hiding this comment.
height prop을 heightClassMap으로 처리하는 방식과 일관성을 맞추고 가독성을 높이기 위해, 폰트 크기에 대해서도 비슷한 맵을 만드는 것을 고려해보세요. 이렇게 하면 컴포넌트의 스타일링 로직이 더 선언적이 되고 유지보수가 쉬워집니다.
먼저 heightClassMap 근처에 fontSizeClassMap을 정의합니다:
const fontSizeClassMap: Record<NonNullable<SearchBarProps['fontSize']>, string> = {
base: 'text-base placeholder:text-base',
lg: 'text-lg placeholder:text-lg'
}그런 다음, 아래 제안과 같이 이 부분을 단순화할 수 있습니다. 명확성을 위해 fontSize prop에 기본값(예: fontSize = 'base')을 추가하는 것도 고려해볼 수 있습니다.
| fontSize === 'lg' | |
| ? 'text-lg placeholder:text-lg' | |
| : 'text-base placeholder:text-base' | |
| fontSizeClassMap[fontSize ?? 'base'] |
|
✅ Syncing Preview App Succeeded Application: |
| <Input | ||
| placeholder="Search" | ||
| sizeVariant="sm" | ||
| className="rounded-full px-8 placeholder:text-[#8A8A8A]" | ||
| className={cn( | ||
| 'rounded-full px-8 placeholder:text-[#C4C4C4]', | ||
| heightClassMap[height], | ||
| fontSize === 'lg' | ||
| ? 'text-lg placeholder:text-lg' | ||
| : 'text-base placeholder:text-base' |
There was a problem hiding this comment.
제가 Input 컴포넌트를 작업했었는데, sizeVariant(sm, md, lg)로 설정하면 높이 설정이 됩니다.
똑같이 36, 40, 46px로 같아서 활용하면 좋을 거 같습니다!
그리고 폰트도 적용해야 할 거 같습니다. 피그마 상에서는 lg(46px)이라고 폰트 사이즈가 더 커지진 않아 보입니다.
seoeun9
left a comment
seoeun9
left a comment
There was a problem hiding this comment.
괜찮으시다면 이닛 쪽도 포함해 컴포넌트화 해주실 수 있나요? course list, assignment 리스트 등에서 사용되고 있고, 로직이 조금 다른듯한데 통일하면 좋을 거 같습니다! (유자차는 엔터 후 필터링, 이닛은 실시간 필터링)
3 participants
Description
서치바를 피그마의 규격에 맞춰 컴포넌트화했습니다.
Additional context
Before submitting the PR, please make sure you do the following
fixes #123).