[Feature/#52] 공통 버튼에 스타일 개선 - #53
Conversation
|
Warning Rate limit exceeded
⌛ 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)
📝 WalkthroughWalkthroughUpdated the Button component and its Storybook: added "danger" and "dangerSoft" variants, adjusted variant typings and styling (colors, borders, opacity, transitions), removed a root Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
> [!CAUTION]
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)src/components/common/button/Button.tsx (1)
59-64:⚠️ Potential issue | 🟠 Major로딩 상태에
aria-busy속성이 누락되어 있습니다.
isLoading이true일 때 버튼은 HTMLdisabled속성을 받고 스피너가 렌더링되지만, 스크린 리더는 버튼이 로딩 중인지 단순히 비활성화된 건지 구분할 수 없습니다.aria-busy={isLoading}속성을 추가하면 보조 기술이 로딩 상태를 인식할 수 있습니다.As per coding guidelines,
src/**파일은 "접근성: 시맨틱 HTML, ARIA 속성 사용 확인"을 준수해야 합니다.♿ 제안 수정
<button className={twMerge(...)} disabled={disabled || isLoading} + aria-busy={isLoading} {...rest} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/common/button/Button.tsx` around lines 59 - 64, The Button component currently sets disabled and renders a spinner when isLoading is true but lacks ARIA to announce loading; update the root button element in Button.tsx (the element using disabled={disabled || isLoading} and {...rest}) to include aria-busy={isLoading}, ensuring you add the attribute in the explicit props (or place it after {...rest} so it cannot be accidentally overridden) so assistive tech can detect the loading state; keep the rest of the logic (isLoading, disabled, spinner span) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/common/button/Button.tsx`:
- Around line 37-43: The variant class strings in Button.tsx include
"disabled:hover:opacity-100" which overrides the component-level conditional
"(disabled || isLoading) && \"opacity-50\"" on hover; update all occurrences of
"disabled:hover:opacity-100" in the variant definitions (the variant map used by
the Button component) to "disabled:hover:opacity-50" so the disabled hover state
preserves the intended 50% opacity for both disabled and loading states.
- Around line 44-45: The dangerSoft variant string lacks a disabled-hover
override, so disabled buttons still respond to hover; update the dangerSoft
class (the dangerSoft entry in the Button variant definitions) to include the
same disabled-hover override used elsewhere (e.g., add
disabled:hover:bg-transparent or the appropriate disabled:hover:opacity-* token
used by other variants) so that when the button is disabled its hover background
change (hover:bg-status-red/20) is suppressed.
---
Outside diff comments:
In `@src/components/common/button/Button.tsx`:
- Around line 59-64: The Button component currently sets disabled and renders a
spinner when isLoading is true but lacks ARIA to announce loading; update the
root button element in Button.tsx (the element using disabled={disabled ||
isLoading} and {...rest}) to include aria-busy={isLoading}, ensuring you add the
attribute in the explicit props (or place it after {...rest} so it cannot be
accidentally overridden) so assistive tech can detect the loading state; keep
the rest of the logic (isLoading, disabled, spinner span) unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/common/button/Button.stories.tsxsrc/components/common/button/Button.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/common/button/Button.tsx`:
- Line 48: The gradient variant in Button (Button.tsx) currently uses
"hover:opacity-90" which overrides the disabled opacity; update the gradient
variant string to include "disabled:hover:opacity-50" (matching the other
opacity-based variants like primary/secondary/danger) so a disabled gradient
button cannot be raised to 90% on hover and retains the disabled visual cue;
locate the gradient class entry in the variant definitions inside the Button
component and append the disabled:hover:opacity-50 token alongside
hover:opacity-90.
- Line 40: The secondary button variant in Button.tsx uses the class string
containing "bg-gray-100 text-brand-900 hover:bg-gray-200 disabled:bg-bg-disabled
disabled:text-text-disabled disabled:hover:opacity-50" but is missing the
explicit "disabled:hover:bg-bg-disabled" override; update the secondary
variant's class list to include disabled:hover:bg-bg-disabled so disabled state
wins over hover (matching the outline and dangerSoft patterns) and prevents
hover:bg-gray-200 from applying when disabled.
|
P4: 확인했습니다! 감사합니다 👍🏻 |
3 participants
🚨 관련 이슈
Closed #52
✨ 변경사항
✏️ 작업 내용
😅 미완성 작업
N/A
📢 논의 사항 및 참고 사항
지금 워크스페이스 관련 페이지 개발하다가 버튼 컴포넌트 사용이 불편한것같아서 수정작업 진행했습니다! 추가로 팀원들 각자 페이지 구현하다가 불편한 스토리북 컴포넌트 발견하면 알려주세요! 바로 수정작업 들어가겠습니다!
Summary by CodeRabbit
New Features
스타일 업데이트