feat(notification-badge): add min width/height/aspect-ratio for size=large#1500
Merged
feat(notification-badge): add min width/height/aspect-ratio for size=large#1500
Conversation
…large Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 54d5f31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
Contributor
Alpha Preview (Stackflow SPA)
|
Contributor
Alpha Preview (Storybook)
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughNotificationBadge의 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/stories/NotificationBadge.stories.tsx (2)
28-67: 타입과 컴포넌트의 이름 충돌로 가독성 저하Line 28에서 정의한 타입
NotificationBadgeCase와 Line 58에서 선언한 컴포넌트NotificationBadgeCase가 동일한 식별자를 사용합니다. TypeScript는 타입과 값의 네임스페이스가 분리되어 있어 컴파일은 되지만, 파라미터 타입({ attach, size, label }: NotificationBadgeCase)이 같은 이름의 컴포넌트를 가리키는지 타입을 가리키는지 읽는 사람 입장에서 혼란스럽습니다. 컴포넌트 이름을NotificationBadgeCaseRenderer나BadgeCase등으로 변경하는 것을 권장합니다.♻️ 제안 diff
-const NotificationBadgeCase = ({ attach, size, label }: NotificationBadgeCase) => { +const NotificationBadgeCaseRenderer = ({ attach, size, label }: NotificationBadgeCase) => { return ( <Box as="span" display="inline-flex" position="relative"> ... </Box> ); }; @@ <VariantTable - Component={NotificationBadgeCase} + Component={NotificationBadgeCaseRenderer}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/stories/NotificationBadge.stories.tsx` around lines 28 - 67, The type and the React component both use the identifier NotificationBadgeCase which harms readability; rename the component (for example to NotificationBadgeCaseRenderer or BadgeCase) and update its declaration and all usages accordingly, keeping the props type annotation as the existing NotificationBadgeCase type (e.g., function NotificationBadgeCaseRenderer(props: NotificationBadgeCase) { ... }) so the type/value namespaces remain clear and other references (imports/exports, story references) are updated to the new component name.
35-41: 폰트 배경의text / large케이스를 스토리에 추가 권장이 PR의 목적인
size=large의 최소 너비/종횡비 보장을 검증하기 위해, 현재 누락된attach: "text"와size: "large"조합을 스토리 케이스에 추가하는 것이 좋습니다. 이 조합은 레시피 레벨에서 완벽하게 지원되며(compoundVariants에 포함), CSS 스타일도 정의되어 있습니다. 폰트 배경에서의 배지 크기/위치 보장을 확인하기 위해 회귀 검증 범위를 넓히길 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/stories/NotificationBadge.stories.tsx` around lines 35 - 41, Add a new Storybook story in NotificationBadge.stories.tsx to cover the missing combination attach: "text" with size: "large" so the visual regression verifies min-width/aspect-ratio for large text-attach badges; update the stories array (or add a new exported story function) that renders the NotificationBadge component with props attach="text" and size="large" (and any representative content/icon) to match the compoundVariant behavior already defined in the recipe/CSS.packages/rootage/components/notification-badge.yaml (1)
13-27: 세 속성에 동일한 설명이 중복되어 있습니다.
minWidth,minHeight,minAspectRatio세 속성 모두 같은 문장을 그대로 반복하고 있어, 문구를 고칠 때 세 곳을 모두 수정해야 하는 유지보수 부담이 생깁니다. 각 속성의 역할(최소 너비/최소 높이/최소 종횡비)을 구분하여 간결하게 기술하거나, 공통 의도는 한 속성(예:minAspectRatio)에만 자세히 남기고 나머지는 짧게 참조하는 방식이 더 깔끔합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rootage/components/notification-badge.yaml` around lines 13 - 27, The three properties minWidth, minHeight, and minAspectRatio repeat the same long description; refactor descriptions to avoid duplication by giving each a concise, specific sentence (minWidth: describe minimum width guarantee; minHeight: describe minimum height guarantee; minAspectRatio: describe minimum aspect-ratio constraint and keep the full shared rationale here) or keep the full shared rationale only on minAspectRatio and make minWidth/minHeight reference it briefly (e.g., "See minAspectRatio for shared sizing rationale"). Update the YAML descriptions for symbols minWidth, minHeight, and minAspectRatio accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/stories/NotificationBadge.stories.tsx`:
- Around line 28-67: The type and the React component both use the identifier
NotificationBadgeCase which harms readability; rename the component (for example
to NotificationBadgeCaseRenderer or BadgeCase) and update its declaration and
all usages accordingly, keeping the props type annotation as the existing
NotificationBadgeCase type (e.g., function NotificationBadgeCaseRenderer(props:
NotificationBadgeCase) { ... }) so the type/value namespaces remain clear and
other references (imports/exports, story references) are updated to the new
component name.
- Around line 35-41: Add a new Storybook story in NotificationBadge.stories.tsx
to cover the missing combination attach: "text" with size: "large" so the visual
regression verifies min-width/aspect-ratio for large text-attach badges; update
the stories array (or add a new exported story function) that renders the
NotificationBadge component with props attach="text" and size="large" (and any
representative content/icon) to match the compoundVariant behavior already
defined in the recipe/CSS.
In `@packages/rootage/components/notification-badge.yaml`:
- Around line 13-27: The three properties minWidth, minHeight, and
minAspectRatio repeat the same long description; refactor descriptions to avoid
duplication by giving each a concise, specific sentence (minWidth: describe
minimum width guarantee; minHeight: describe minimum height guarantee;
minAspectRatio: describe minimum aspect-ratio constraint and keep the full
shared rationale here) or keep the full shared rationale only on minAspectRatio
and make minWidth/minHeight reference it briefly (e.g., "See minAspectRatio for
shared sizing rationale"). Update the YAML descriptions for symbols minWidth,
minHeight, and minAspectRatio accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8d0638f1-4948-49f7-8c93-74854619fa29
⛔ Files ignored due to path filters (11)
docs/public/rootage/components/notification-badge.jsonis excluded by!**/public/**/*packages/css/all.cssis excluded by!packages/css/**/*packages/css/all.layered.cssis excluded by!packages/css/**/*packages/css/all.layered.min.cssis excluded by!packages/css/**/*packages/css/all.min.cssis excluded by!packages/css/**/*packages/css/recipes/notification-badge.cssis excluded by!packages/css/**/*packages/css/recipes/notification-badge.layered.cssis excluded by!packages/css/**/*packages/css/vars/component/notification-badge.d.tsis excluded by!packages/css/**/*packages/css/vars/component/notification-badge.mjsis excluded by!packages/css/**/*packages/qvism-preset/src/vars/component/notification-badge.d.tsis excluded by!packages/qvism-preset/src/vars/**/*packages/qvism-preset/src/vars/component/notification-badge.mjsis excluded by!packages/qvism-preset/src/vars/**/*
📒 Files selected for processing (4)
.changeset/shiny-suits-pick.mddocs/stories/NotificationBadge.stories.tsxpackages/qvism-preset/src/recipes/notification-badge.tspackages/rootage/components/notification-badge.yaml
Contributor
Alpha Preview (Docs)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
릴리스 노트
버그 수정
문서