-
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] Color 추가 #26
[FEAT] Color 추가 #26
Conversation
Walkthrough이 변경 사항은 여러 컴포넌트의 스타일을 수정하는 내용을 포함합니다. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Update: 2024년 10월 26일 03시 41분 26초 |
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/styles/theme/colors.ts (1)
8-10
: 색상 사용에 대한 문서화를 추가하는 것이 좋겠습니다.각 그레이 스케일 값의 용도와 적용 가이드라인을 주석으로 추가하면 다른 개발자들이 더 쉽게 이해하고 활용할 수 있을 것 같습니다.
다음과 같이 주석을 추가하는 것을 제안합니다:
gray: { + // 배경색, 비활성 상태의 엘리먼트에 사용 + // 입력 필드, 경계선에 사용 + // 버튼, 구분선에 사용src/components/common/Button/Button.variants.ts (1)
Line range hint
3-3
: TODO 주석에 대한 작업이 필요합니다.hover와 disabled 상태의 색상 추가에 대한 TODO 주석이 있습니다. 현재 구현에서는 이미 hover와 disabled 상태가 추가되어 있으므로, 이 주석을 제거하거나 추가로 필요한 작업이 있다면 명확히 기술해주세요.
추가 작업이 필요하시다면 도움을 드릴 수 있습니다. GitHub 이슈를 생성해드릴까요?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/assets/icons/marker.svg
is excluded by!**/*.svg
📒 Files selected for processing (4)
- src/components/common/Button/Button.variants.ts (1 hunks)
- src/components/common/Input/index.tsx (1 hunks)
- src/components/common/Marker/index.tsx (1 hunks)
- src/styles/theme/colors.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/common/Input/index.tsx
🔇 Additional comments (5)
src/styles/theme/colors.ts (1)
8-10
: 그레이 스케일 값의 변경이 적절해 보입니다.그레이 스케일의 단계가 더 세분화되어 디자인 시스템의 유연성이 향상되었습니다. 기존 색상값들의 논리적인 진행이 잘 유지되었습니다.
src/components/common/Marker/index.tsx (3)
11-12
: 상대적 위치 지정으로 인한 레이아웃 안정성 향상
relative
클래스를 추가하여 마커의 위치 지정 방식을 개선한 것은 좋은 변경입니다. 이는 레이아웃의 안정성을 향상시킬 것으로 예상됩니다.
13-16
: 아이콘 위치 하드코딩에 대한 유연성 검토 필요이전의 동적 위치 결정 로직을 제거하고 하드코딩된 위치값을 사용하는 것이 유연성을 저해할 수 있습니다. 다양한 카테고리에 따라 다른 위치가 필요한 경우를 고려해야 할 수 있습니다.
#!/bin/bash # 카테고리별 특별한 위치 지정이 필요한지 확인 rg "categoryName" src/또한, 반응형 디자인 관점에서 하드코딩된 45% 값이 모든 화면 크기에서 적절한지 검토가 필요합니다.
7-7
: 아이콘 크기 변경이 디자인 일관성에 미치는 영향 검토 필요아이콘 크기를 35에서 30으로 줄이는 것이 전체적인 디자인 시스템의 일관성과 사용자 경험에 영향을 미칠 수 있습니다. 이 변경이 디자인 가이드라인과 일치하는지 확인해주세요.
✅ Verification successful
아이콘 크기 변경은 디자인 시스템과 일치합니다
검증 결과, Icon 컴포넌트의 기본 크기가 30px로 설정되어 있어 Marker 컴포넌트의 ICON_SIZE를 30px로 변경한 것은 디자인 시스템의 일관성을 개선하는 변경입니다. 이는 다음과 같은 이유로 적절합니다:
- Icon 컴포넌트의 기본 크기(size = 30)와 일치하여 디자인 일관성 확보
- 다른 컴포넌트들의 아이콘 사용 패턴과 조화를 이룸
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 다른 컴포넌트에서 사용되는 아이콘 크기 확인 rg -l "size=" src/components/Length of output: 797
Script:
#!/bin/bash # 아이콘 크기 관련 상수 및 속성 검색 rg -g "*.{tsx,ts,js,jsx}" "size.*=.*\d+" src/components/ # MarkerIcon 컴포넌트 사용 검색 rg -g "*.{tsx,ts,js,jsx}" "MarkerIcon" src/components/ # 디자인 시스템 관련 상수 검색 rg -g "*.{tsx,ts,js,jsx}" "(ICON|icon).*SIZE" src/Length of output: 867
src/components/common/Button/Button.variants.ts (1)
11-11
: gray 변형의 스타일 변경이 적절해 보입니다.gray-150 클래스를 사용하여 배경색을 변경하고, hover 상태를 추가한 것이 디자인 시스템과 잘 어울립니다.
primary: | ||
'bg-primary text-white hover:bg-[#FF3131] disabled:opacity-50 disabled:cursor-not-allowed', |
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.
하드코딩된 색상값을 테마 토큰으로 변경해주세요.
hover 상태의 색상값이 직접 하드코딩되어 있습니다. 테마 시스템의 일관성을 위해 색상 토큰을 사용하는 것이 좋습니다.
다음과 같이 수정해주세요:
-'bg-primary text-white hover:bg-[#FF3131] disabled:opacity-50 disabled:cursor-not-allowed',
+'bg-primary text-white hover:bg-primary-dark disabled:opacity-50 disabled:cursor-not-allowed',
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
primary: | |
'bg-primary text-white hover:bg-[#FF3131] disabled:opacity-50 disabled:cursor-not-allowed', | |
primary: | |
'bg-primary text-white hover:bg-primary-dark disabled:opacity-50 disabled:cursor-not-allowed', |
관련 이슈
close #24
📑 작업 내용
💬 리뷰 중점 사항/기타 참고 사항
Summary by CodeRabbit