refactor(card): add custom ratio for better flexibility#101
Conversation
froggy1014
requested review from
SEMIN-97,
bae-sh,
developerjhp,
heeji289,
hy57in,
kimdaeyeob,
noahluftyang,
synuns and
yeojini
as code owners
🦋 Changeset detectedLatest commit: 39ef0a5 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 |
|
Warning Rate limit exceeded@froggy1014 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
워크스루이 풀 리퀘스트는 변경 사항
관련 가능성 있는 PR
제안된 리뷰어
토끼의 시 🐰
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/eighty-apricots-punch.md (1)
1-5: 변경사항 설명을 더 자세히 작성해주세요현재 설명이 너무 간단합니다. 다음과 같은 내용을 포함하면 좋을 것 같습니다:
- 커스텀 비율 기능의 사용 사례
- 기존 비율과의 차이점
- 마이그레이션 가이드 (필요한 경우)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/eighty-apricots-punch.md(1 hunks)package.json(1 hunks)packages/card/src/Card.stories.tsx(1 hunks)packages/card/src/Card.tsx(3 hunks)
🔇 Additional comments (4)
packages/card/src/Card.stories.tsx (1)
18-18: 기본 비율 변경에 대한 검토가 필요합니다기본 스토리의 비율이 'rectangle'에서 'portrait'로 변경되었습니다. 이는 커스텀 비율 기능 추가라는 PR의 주요 목적과 맞지 않아 보입니다. 'custom' 비율을 보여주는 새로운 스토리를 추가하는 것이 더 적절할 것 같습니다.
packages/card/src/Card.tsx (2)
12-12: 타입 정의가 적절해 보입니다CardRatio 타입에 'custom' 옵션을 추가한 것이 적절합니다.
24-24: 기본값 변경에 대한 주의가 필요합니다기본값을 'custom'으로 변경하는 것은 기존 사용자에게 영향을 줄 수 있습니다. 다음 사항을 고려해주세요:
- 기존 코드의 호환성
- 마이그레이션 가이드 필요성
- 기본값으로 'rectangle' 유지 검토
package.json (1)
62-62: Markdown 파일 린트 제외에 대한 검토가 필요합니다lint-staged 설정에서 Markdown 파일이 제외되었습니다. 이는 다음과 같은 영향을 줄 수 있습니다:
- 문서 포맷팅 일관성 저하
- 문서 품질 관리의 어려움
가능하다면 Markdown 파일을 린트 대상에 포함하는 것을 고려해주세요.
packages/card/src/Card.tsx
Outdated
| case 'custom': | ||
| return 'auto'; | ||
| default: | ||
| return '16 / 9'; | ||
| return 'auto'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
getAspectRatio 함수의 개선이 필요합니다
'custom'과 default case 모두 'auto'를 반환하고 있습니다. 다음과 같은 개선을 제안합니다:
- 'custom' 케이스에서 사용자 정의 비율을 받을 수 있도록 함수 시그니처 수정
- default case에서 적절한 fallback 값 설정
예시 구현:
-function getAspectRatio(ratio: CardRatio) {
+function getAspectRatio(ratio: CardRatio, customRatio?: string) {
switch (ratio) {
case 'square':
return '1 / 1';
case 'rectangle':
return '16 / 9';
case 'wide':
return '21 / 9';
case 'portrait':
return '3 / 4';
case 'custom':
- return 'auto';
+ return customRatio ?? 'auto';
default:
- return 'auto';
+ return '16 / 9';
}
}Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/card/src/Card.test.tsx (2)
40-53: 비율 관련 테스트 케이스 보완이 필요합니다현재 테스트는 기본적인 케이스만 다루고 있습니다. 다음과 같은 추가 테스트 케이스가 필요해 보입니다:
- 잘못된 ratio 값이 전달되었을 때의 동작
- ratio prop이 undefined일 때의 동작
예시 테스트 케이스:
test('ratio prop이 없을 때 기본값으로 rectangle이 적용된다.', () => { render(<Card>Card</Card>); expect(screen.getByText('Card')).toHaveStyle('aspect-ratio: 16 / 9'); }); test('잘못된 ratio 값이 전달되면 기본값이 적용된다.', () => { render(<Card ratio={'invalid' as CardRatio}>Card</Card>); expect(screen.getByText('Card')).toHaveStyle('aspect-ratio: 16 / 9'); });
69-81: variant 배경색 테스트의 가독성 개선이 필요합니다현재 테스트 설명에 템플릿 리터럴로 색상 값이 포함되어 있어 가독성이 떨어집니다.
다음과 같이 개선하는 것을 제안드립니다:
- test(`variant가 filled일 때 배경색이 ${color.gray100}이다.`, () => { + test('variant가 filled일 때 올바른 배경색이 적용된다.', () => { - test(`variant가 outline일 때 배경색이 ${color.gray50}이다.`, () => { + test('variant가 outline일 때 올바른 배경색이 적용된다.', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/card/src/Card.test.tsx(2 hunks)packages/card/src/Card.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: chromatic
🔇 Additional comments (2)
packages/card/src/Card.tsx (2)
79-79:⚠️ Potential issuegetAspectRatio의 default case 처리 방식 개선이 필요합니다
default case에서 'auto'를 반환하는 것은 예측하기 어려운 동작을 초래할 수 있습니다. 이는 이전 버전과의 호환성 문제도 발생시킬 수 있습니다.
다음과 같은 개선을 제안드립니다:
default: - return 'auto'; + return '16 / 9'; // 이전 버전과의 호환성 유지Likely invalid or redundant comment.
12-12: CardRatio 타입에 'auto' 추가에 대한 검토가 필요합니다'auto' 값의 추가는 좋은 개선이지만, 이전 리뷰에서 논의된 'custom' 비율 지원에 대한 요구사항이 해결되지 않았습니다.
사용자 정의 비율을 지원하기 위해 다음과 같은 접근 방식을 고려해보시는 것은 어떨까요?
export type CardRatio = | 'rectangle' | 'square' | 'wide' | 'portrait' | 'auto' | { width: number; height: number };✅ Verification successful
'auto' 타입 추가는 적절하며, 커스텀 비율은 향후 개선사항으로 고려하시면 좋겠습니다
현재 구현된 'auto' 옵션은 기존 기능을 해치지 않으면서 유연성을 제공합니다. 커스텀 비율 지원은 좋은 제안이지만, 이는 별도의 기능 개선으로 다루는 것이 적절해 보입니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find all usages of CardRatio rg "CardRatio" -B 2 -A 2 # Check if there was any previous custom ratio implementation ast-grep --pattern 'type CardRatio = $$$' # Look for ratio-related logic in Card component ast-grep --pattern 'function getAspectRatio($$$) { $$$ }'Length of output: 1692
|
LGTM 👍 |
5 participants
Changes
기존에 반드시
aspect-ratio가 default로 들어가는 문제가 있어서 사용자가 Card 컴포넌트로 UI를 좀 더 유연하게 구현하기 힘들다는 피드백이 있어서 이에 수정합니다.Visuals
Checklist
Additional Discussion Points
Summary by CodeRabbit
새로운 기능
변경 사항
테스트