[FEAT] WTH-171 출석 페이지 컴포넌트 구현#11
Conversation
PR 검증 결과✅ TypeScript: 통과 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough새로운 클라이언트 측 출석 페이지와 여러 UI 컴포넌트가 추가됩니다. AttendancePage는 AttendanceProgressBar, 카드 기반 출석 확인 UI 및 QR 기반 다이얼로그 예제를 포함하고, Dialog는 Header/Body/Footer로 구조가 재정비되며 Card 및 Divider 컴포넌트가 도입됩니다. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AttendancePage
participant Card
participant Dialog
participant AttendanceProgressBar
User->>AttendancePage: 페이지 접근
AttendancePage->>AttendanceProgressBar: 출석률 데이터로 렌더링 요청
AttendancePage->>Card: 출석 확인 카드 표시
User->>Card: "출석 확인" 클릭
Card->>Dialog: DialogTrigger 열기
Dialog->>User: QR 모달 표시 (이미지/온보딩 페이지 포함)
User->>Dialog: 모달 내 네비게이션/닫기 상호작용
Dialog->>AttendancePage: 닫힘 이벤트(옵션)
AttendancePage->>AttendanceProgressBar: 상태 업데이트(필요 시)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
구현한 기능 Preview: https://weeth-2h26mv854-weethsite-4975s-projects.vercel.app |
PR 검증 결과✅ TypeScript: 통과 🎉 모든 검증을 통과했습니다! |
|
구현한 기능 Preview: https://weeth-f77vtqy88-weethsite-4975s-projects.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (3)
src/stores/modal-store.ts (1)
3-13: 단일isOpen불리언 구조는 이론상 여러 모달 사용 시 충돌 가능하지만, 현재 코드는 제어 모드로 안전하게 구현되어 있습니다.Modal 컴포넌트가 이미 제어/비제어 이중 모드를 지원하므로, 여러 모달이 필요할 때는 제어 모드(
openprop과onOpenChange)를 사용하여 충돌을 방지할 수 있습니다. AttendancePage에서 보이듯 첫 번째 모달만 store 기반(비제어)이고 나머지 4개는 모두 로컬 상태로 제어되고 있어 현재 설계는 적절히 활용되고 있습니다. 다만 API 설계 관점에서, 여러 모달 ID 기반 상태(Record<string, boolean>등)로 리팩터링하면 향후 개발자 실수를 방지할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/modal-store.ts` around lines 3 - 13, The current ModalStore (interface ModalStore and useModalStore with isOpen, open, close) uses a single boolean which can collide if multiple modals are used; change the store to track modal visibility by id (e.g., state: Record<string, boolean>), update the API to accept an id parameter (e.g., isOpen(id), open(id), close(id), toggle(id)) and adjust useModalStore implementation accordingly so callers can control individual modals by id (keep backward compatibility by providing a default id if needed or migrate callers like AttendancePage to controlled usage with ids).src/app/(public)/attendance/page.tsx (2)
28-28: 데모 로그는 실제 액션 핸들러로 교체하거나 TODO로 분리해 주세요.Line 28의
console.log는 릴리즈 시 디버그 흔적으로 남기 쉽습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(public)/attendance/page.tsx at line 28, Replace the demo console.log in the onSecondaryClick prop with a real action handler or a clear TODO marker: remove the inline console.log('출석코드 확인') and instead call a named handler (e.g., handleSecondaryClick) defined in the same component (or add a TODO comment and a no-op handler) so there are no stray console.debug statements in production; update the onSecondaryClick prop to reference that handler and implement the desired behavior (open modal, navigate, copy code, etc.) inside handleSecondaryClick.
19-19: 간격 토큰 규칙에 맞춰gap값을 변경해 주세요.Line 19의
gap-6은 spacing 토큰 규칙 범위(gap-100~400) 밖입니다.As per coding guidelines "Use spacing design tokens: `p-100~500` for padding, `gap-100~400` for gaps".🎯 제안 수정안
- <div className="flex flex-col gap-6 px-450"> + <div className="flex flex-col gap-300 px-450">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(public)/attendance/page.tsx at line 19, The gap utility on the container div uses a non-conforming token `gap-6`; update the className on the div in attendance/page.tsx (the element with className="flex flex-col gap-6 px-450") to use an approved spacing token within gap-100..gap-400 (for example replace `gap-6` with `gap-100` or another appropriate token like `gap-200`) so it follows the spacing design-token rule.
🤖 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/app/`(public)/attendance/page.tsx:
- Around line 157-207: The modal's onOpenChange currently calls
setOnboardingModalOpen directly so closing doesn't reset the wizard page;
replace that with a handler that calls setOnboardingModalOpen(open) and when
open === false also calls setCurrentPage(1) so the onboarding always starts at
page 1 on reopen—look for the Modal component using props onboardingModalOpen,
setOnboardingModalOpen, currentPage and setCurrentPage and update its
onOpenChange accordingly.
In `@src/components/attendance/AttendanceProgressBar.tsx`:
- Around line 13-24: The progress bar is missing accessibility attributes and is
vulnerable to NaN values; in AttendanceProgressBar ensure attendanceRate is
coerced to a valid number (e.g., const attendanceNum = Number(attendanceRate) ||
0) then compute safeAttendanceRate = Math.min(100, Math.max(0, attendanceNum))
and absenceRate = 100 - safeAttendanceRate, and update the progress indicator
div (the element using safeAttendanceRate for width) to include
role="progressbar", aria-valuemin="0", aria-valuemax="100",
aria-valuenow={safeAttendanceRate} and either aria-label or aria-labelledby so
screen readers can announce it; also use safeAttendanceRate for any inline style
width to avoid width: NaN% issues.
In `@src/components/ui/ArrowRightIcon.tsx`:
- Line 15: Replace the hardcoded color class in ArrowRightIcon's className
expression (currently using "text-neutral-800" in the cn(...) call) with the
appropriate design token, e.g., "text-text-normal" or "text-text-strong" as the
icon's default; keep the existing className prop merged (cn('text-text-normal',
className)) so consumers can still override the color while preserving theme
tokens across light/dark themes.
In `@src/components/ui/card.tsx`:
- Around line 13-16: The style variants 'default', 'onlyText', and 'buttonSet'
in src/components/ui/card.tsx use raw utility classes (e.g., gap-6, py-6, px-6,
text-sm, text-muted-foreground); replace these with the design tokens mandated
by the project (spacing tokens p-100…p-500 and gap-100…gap-400 for padding/gaps,
and typography tokens typo-h1…typo-caption2 for text) so the Card component uses
token-based classes; update all other occurrences mentioned (around the ranges
referenced) to use the corresponding token names instead of raw utilities while
preserving the same visual intent.
- Around line 55-61: The card div (data-slot="card") uses role="button" but
lacks keyboard activation; add an onKeyDown handler in the Card component that
listens for Enter (Enter/Return) and Space keys and invokes the existing onClick
prop (or onActivate) so keyboard users can trigger the card; for Space call
event.preventDefault() before invoking onClick to avoid unwanted scrolling, and
ensure the handler gracefully handles undefined onClick and preserves other
props spread onto the element.
In `@src/components/ui/CloseIcon.tsx`:
- Around line 1-24: The CloseIcon component file is failing CI due to Prettier
formatting; run Prettier (e.g., prettier --write src/components/ui/CloseIcon.tsx
or your repo's format script) to reformat the file, re-stage the modified file,
and push the commit so CI can re-run; the change will affect the CloseIcon
function and its import of cn—no code logic changes required, just apply the
auto-format and commit.
- Line 15: In the CloseIcon component, remove the hardcoded cursor-pointer and
the non-token color class from the default class list: replace
className={cn('text-neutral-800 cursor-pointer', className)} with a design-token
color (e.g., 'text-text-strong') and omit cursor styles so the click cursor is
applied by the interactive wrapper (button) instead; keep using the cn utility
and the className prop so callers can override/add classes as needed.
In `@src/components/ui/modal/Modal.tsx`:
- Around line 1-68: Prettier formatting errors in Modal.tsx are causing CI
failures; run the project's Prettier config (or npm/yarn format script) and
reformat this file so it conforms to the code style, then commit the result.
Focus on normalizing import/quote spacing, consistent semicolons, indentation,
and JSX formatting in the Modal component (function Modal, createPortal return
JSX, and the useEffect/useCallback blocks such as handleClose) so that
lint/format checks pass. Ensure no logic changes are made—only formatting
adjustments—and re-run the formatter/CI to verify the pipeline succeeds.
- Around line 7-15: Add a className prop to ModalProps and accept it in the
Modal component signature (e.g., className?: string), then pass that className
through to the modal's root/container element (the element rendered inside
Modal, referenced by the container variable or JSX around where children are
rendered) so callers can customize styling; ensure the prop is forwarded and
does not override existing class composition logic (merge with existing classes
if any) and keep the existing open/onOpenChange behavior intact.
- Around line 50-61: The modal markup lacks proper ARIA dialog attributes and
keyboard dismissal handling; update the Modal component wrapper (the element
containing children) to include role="dialog" and aria-modal="true", provide an
accessible label via aria-labelledby or aria-label (connect to a heading inside
the modal or add a descriptive prop), ensure focus is trapped/focused into the
dialog on open and returned on close, and add Escape key handling that calls
handleClose to close the dialog; use the existing handleClose function and the
modal container element to implement these accessibility fixes.
In `@src/components/ui/modal/ModalBody.tsx`:
- Around line 6-22: ModalBody lacks the shared UI pattern: define variants with
cva and include VariantProps and asChild support; update the ModalBodyProps to
extend React.HTMLAttributes<HTMLDivElement> & VariantProps<typeof
modalBodyVariants> and add an optional asChild?: boolean, create a
modalBodyVariants using cva (e.g., modalBodyVariants) and use
cn(modalBodyVariants({ ...propsVariantOptions }), className) to compute classes,
and render using Radix's Slot when asChild is true (or default to div) so the
component supports variant + asChild while keeping the exported ModalBody and
ModalBodyProps identifiers.
In `@src/components/ui/modal/ModalFooter.tsx`:
- Around line 25-27: Replace the arbitrary gap value and split the inline
ternary: in ModalFooter.tsx (ModalFooter component) change className "flex
flex-col gap-[10px] p-400" to use a spacing token like "flex flex-col gap-200
p-400", and refactor the JSX so pagination and description are rendered in
separate expressions — e.g. render {pagination ? pagination : null} followed by
{!pagination && description && <p className="typo-caption2 text-text-alternative
mt-200 text-center">{description}</p>} to preserve the original behavior but
improve readability and token compliance.
In `@src/components/ui/modal/ModalHeader.tsx`:
- Around line 27-49: The close button can be non-functional for controlled
modals because handleClose falls back to useModalStore().close; update
ModalHeader so handleClose does not default to the store close and only render
the CloseIcon when an external onClose is provided: replace "const handleClose =
onClose || close" with "const handleClose = onClose" and change both CloseIcon
render conditions to "showClose && onClose" (referencing ModalHeader,
useModalStore, handleClose, onClose, showClose, CloseIcon) so controlled modals
require the parent-provided onClose to show a working X button.
---
Nitpick comments:
In `@src/app/`(public)/attendance/page.tsx:
- Line 28: Replace the demo console.log in the onSecondaryClick prop with a real
action handler or a clear TODO marker: remove the inline console.log('출석코드 확인')
and instead call a named handler (e.g., handleSecondaryClick) defined in the
same component (or add a TODO comment and a no-op handler) so there are no stray
console.debug statements in production; update the onSecondaryClick prop to
reference that handler and implement the desired behavior (open modal, navigate,
copy code, etc.) inside handleSecondaryClick.
- Line 19: The gap utility on the container div uses a non-conforming token
`gap-6`; update the className on the div in attendance/page.tsx (the element
with className="flex flex-col gap-6 px-450") to use an approved spacing token
within gap-100..gap-400 (for example replace `gap-6` with `gap-100` or another
appropriate token like `gap-200`) so it follows the spacing design-token rule.
In `@src/stores/modal-store.ts`:
- Around line 3-13: The current ModalStore (interface ModalStore and
useModalStore with isOpen, open, close) uses a single boolean which can collide
if multiple modals are used; change the store to track modal visibility by id
(e.g., state: Record<string, boolean>), update the API to accept an id parameter
(e.g., isOpen(id), open(id), close(id), toggle(id)) and adjust useModalStore
implementation accordingly so callers can control individual modals by id (keep
backward compatibility by providing a default id if needed or migrate callers
like AttendancePage to controlled usage with ids).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/assets/icons/arrow-right.svgis excluded by!**/*.svgsrc/assets/icons/delete.svgis excluded by!**/*.svgsrc/assets/icons/dummy.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
src/app/(public)/attendance/page.tsxsrc/components/attendance/AttendanceProgressBar.tsxsrc/components/ui/ArrowRightIcon.tsxsrc/components/ui/CloseIcon.tsxsrc/components/ui/Divider.tsxsrc/components/ui/card.tsxsrc/components/ui/index.tssrc/components/ui/modal/Modal.tsxsrc/components/ui/modal/ModalBody.tsxsrc/components/ui/modal/ModalFooter.tsxsrc/components/ui/modal/ModalHeader.tsxsrc/components/ui/modal/index.tssrc/stores/index.tssrc/stores/modal-store.ts
src/app/(public)/attendance/page.tsx
Outdated
| <Modal open={onboardingModalOpen} onOpenChange={setOnboardingModalOpen}> | ||
| <ModalHeader | ||
| overline="사이트 완성하기" | ||
| title="동아리의 프로필과 배경화면을 설정해서 사이트를 꾸며보세요!" | ||
| description="관리자 서비스에서 언제든 수정할 수 있어요." | ||
| /> | ||
| <ModalBody> | ||
| {/* 이미지 영역 */} | ||
| <div className="relative aspect-video w-full overflow-hidden rounded-lg"> | ||
| <Image src={DummyImage} alt="웹사이트 미리보기" fill className="object-cover" /> | ||
| </div> | ||
| {/* 설정하러 가기 링크 */} | ||
| <p className="typo-body2 text-text-alternative text-center">설정하러 가기</p> | ||
| </ModalBody> | ||
| <ModalFooter | ||
| pagination={ | ||
| <div className="flex items-center justify-center gap-300"> | ||
| <button | ||
| onClick={() => setCurrentPage((prev) => Math.max(1, prev - 1))} | ||
| className="typo-body2 text-text-alternative disabled:opacity-50" | ||
| disabled={currentPage === 1} | ||
| > | ||
| < | ||
| </button> | ||
| <span className="typo-body2 text-text-normal">{currentPage} / 4</span> | ||
| <button | ||
| onClick={() => setCurrentPage((prev) => Math.min(4, prev + 1))} | ||
| className="typo-body2 text-text-alternative disabled:opacity-50" | ||
| disabled={currentPage === 4} | ||
| > | ||
| > | ||
| </button> | ||
| </div> | ||
| } | ||
| > | ||
| <Button | ||
| variant="primary" | ||
| size="lg" | ||
| onClick={() => { | ||
| if (currentPage < 4) { | ||
| setCurrentPage((prev) => prev + 1); | ||
| } else { | ||
| setOnboardingModalOpen(false); | ||
| setCurrentPage(1); | ||
| } | ||
| }} | ||
| > | ||
| 다음 | ||
| </Button> | ||
| </ModalFooter> | ||
| </Modal> |
There was a problem hiding this comment.
온보딩 모달을 외부에서 닫을 때 currentPage가 유지됩니다.
Line 157의 onOpenChange={setOnboardingModalOpen}는 열림 상태만 바꾸고 페이지 초기화는 하지 않아, 재오픈 시 중간 단계에서 시작할 수 있습니다. 닫힘 시점(open === false)에 setCurrentPage(1)도 함께 처리해 주세요.
🧩 제안 수정안
- <Modal open={onboardingModalOpen} onOpenChange={setOnboardingModalOpen}>
+ <Modal
+ open={onboardingModalOpen}
+ onOpenChange={(open) => {
+ setOnboardingModalOpen(open);
+ if (!open) setCurrentPage(1);
+ }}
+ >📝 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.
| <Modal open={onboardingModalOpen} onOpenChange={setOnboardingModalOpen}> | |
| <ModalHeader | |
| overline="사이트 완성하기" | |
| title="동아리의 프로필과 배경화면을 설정해서 사이트를 꾸며보세요!" | |
| description="관리자 서비스에서 언제든 수정할 수 있어요." | |
| /> | |
| <ModalBody> | |
| {/* 이미지 영역 */} | |
| <div className="relative aspect-video w-full overflow-hidden rounded-lg"> | |
| <Image src={DummyImage} alt="웹사이트 미리보기" fill className="object-cover" /> | |
| </div> | |
| {/* 설정하러 가기 링크 */} | |
| <p className="typo-body2 text-text-alternative text-center">설정하러 가기</p> | |
| </ModalBody> | |
| <ModalFooter | |
| pagination={ | |
| <div className="flex items-center justify-center gap-300"> | |
| <button | |
| onClick={() => setCurrentPage((prev) => Math.max(1, prev - 1))} | |
| className="typo-body2 text-text-alternative disabled:opacity-50" | |
| disabled={currentPage === 1} | |
| > | |
| < | |
| </button> | |
| <span className="typo-body2 text-text-normal">{currentPage} / 4</span> | |
| <button | |
| onClick={() => setCurrentPage((prev) => Math.min(4, prev + 1))} | |
| className="typo-body2 text-text-alternative disabled:opacity-50" | |
| disabled={currentPage === 4} | |
| > | |
| > | |
| </button> | |
| </div> | |
| } | |
| > | |
| <Button | |
| variant="primary" | |
| size="lg" | |
| onClick={() => { | |
| if (currentPage < 4) { | |
| setCurrentPage((prev) => prev + 1); | |
| } else { | |
| setOnboardingModalOpen(false); | |
| setCurrentPage(1); | |
| } | |
| }} | |
| > | |
| 다음 | |
| </Button> | |
| </ModalFooter> | |
| </Modal> | |
| <Modal | |
| open={onboardingModalOpen} | |
| onOpenChange={(open) => { | |
| setOnboardingModalOpen(open); | |
| if (!open) setCurrentPage(1); | |
| }} | |
| > | |
| <ModalHeader | |
| overline="사이트 완성하기" | |
| title="동아리의 프로필과 배경화면을 설정해서 사이트를 꾸며보세요!" | |
| description="관리자 서비스에서 언제든 수정할 수 있어요." | |
| /> | |
| <ModalBody> | |
| {/* 이미지 영역 */} | |
| <div className="relative aspect-video w-full overflow-hidden rounded-lg"> | |
| <Image src={DummyImage} alt="웹사이트 미리보기" fill className="object-cover" /> | |
| </div> | |
| {/* 설정하러 가기 링크 */} | |
| <p className="typo-body2 text-text-alternative text-center">설정하러 가기</p> | |
| </ModalBody> | |
| <ModalFooter | |
| pagination={ | |
| <div className="flex items-center justify-center gap-300"> | |
| <button | |
| onClick={() => setCurrentPage((prev) => Math.max(1, prev - 1))} | |
| className="typo-body2 text-text-alternative disabled:opacity-50" | |
| disabled={currentPage === 1} | |
| > | |
| < | |
| </button> | |
| <span className="typo-body2 text-text-normal">{currentPage} / 4</span> | |
| <button | |
| onClick={() => setCurrentPage((prev) => Math.min(4, prev + 1))} | |
| className="typo-body2 text-text-alternative disabled:opacity-50" | |
| disabled={currentPage === 4} | |
| > | |
| > | |
| </button> | |
| </div> | |
| } | |
| > | |
| <Button | |
| variant="primary" | |
| size="lg" | |
| onClick={() => { | |
| if (currentPage < 4) { | |
| setCurrentPage((prev) => prev + 1); | |
| } else { | |
| setOnboardingModalOpen(false); | |
| setCurrentPage(1); | |
| } | |
| }} | |
| > | |
| 다음 | |
| </Button> | |
| </ModalFooter> | |
| </Modal> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`(public)/attendance/page.tsx around lines 157 - 207, The modal's
onOpenChange currently calls setOnboardingModalOpen directly so closing doesn't
reset the wizard page; replace that with a handler that calls
setOnboardingModalOpen(open) and when open === false also calls
setCurrentPage(1) so the onboarding always starts at page 1 on reopen—look for
the Modal component using props onboardingModalOpen, setOnboardingModalOpen,
currentPage and setCurrentPage and update its onOpenChange accordingly.
There was a problem hiding this comment.
페이지네이션은 추후에 컴포넌트로 구현 예정이야 지금은 예시로 넣어둔거야
There was a problem hiding this comment.
@dalzzy, 알겠습니다! 추후 리팩토링 예정이라면 지금은 예시 코드로 두셔도 괜찮습니다.
다만 한 가지만 참고해 주세요 — 제가 지적한 부분은 페이지네이션 컴포넌트 자체보다는 모달을 오버레이나 X 버튼으로 닫았을 때 currentPage 상태가 초기화되지 않는 문제였습니다. 현재 Line 157의 onOpenChange={setOnboardingModalOpen}는 열림 상태만 토글하고 페이지는 리셋하지 않아서, 재오픈 시 중간 페이지에서 시작할 수 있습니다.
예시 코드라도 이 부분은 사용자가 테스트할 때 혼란을 줄 수 있으니, 빠른 시간에 아래처럼 간단히 수정해 두시면 좋을 것 같습니다:
onOpenChange={(open) => {
setOnboardingModalOpen(open);
if (!open) setCurrentPage(1);
}}물론 리팩토링 시점에 함께 처리하셔도 무방합니다! 😊
🧠 Learnings used
Learnt from: CR
Repo: Team-Weeth/weeth-client PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-02T10:01:25.427Z
Learning: Applies to **/*.{ts,tsx} : Use React 19 with TypeScript and Next.js 16 (App Router) as the core tech stack
| // 값 범위 제한 | ||
| const safeAttendanceRate = Math.min(100, Math.max(0, attendanceRate)); | ||
| // 결석률 = 100 - 출석률 | ||
| const absenceRate = 100 - safeAttendanceRate; | ||
|
|
||
| return ( | ||
| <div | ||
| className={cn( | ||
| 'bg-container-neutral-interaction relative h-[15px] w-full overflow-hidden rounded-[4px]', | ||
| className, | ||
| )} | ||
| {...props} |
There was a problem hiding this comment.
프로그레스바 접근성 속성과 비정상 숫자 방어 로직이 필요합니다.
시각적으로는 진행률이 보이지만, role="progressbar"/aria-valuenow 등이 없어 보조기기에서는 현재 상태를 알 수 없습니다. 또한 NaN 입력 시 width: NaN%가 됩니다.
♿ 제안 수정
- const safeAttendanceRate = Math.min(100, Math.max(0, attendanceRate));
+ const safeAttendanceRate = Number.isFinite(attendanceRate)
+ ? Math.min(100, Math.max(0, attendanceRate))
+ : 0;
return (
<div
+ role="progressbar"
+ aria-valuemin={0}
+ aria-valuemax={100}
+ aria-valuenow={safeAttendanceRate}
className={cn(
'bg-container-neutral-interaction relative h-[15px] w-full overflow-hidden rounded-[4px]',
className,
)}
{...props}
>Also applies to: 29-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/attendance/AttendanceProgressBar.tsx` around lines 13 - 24,
The progress bar is missing accessibility attributes and is vulnerable to NaN
values; in AttendanceProgressBar ensure attendanceRate is coerced to a valid
number (e.g., const attendanceNum = Number(attendanceRate) || 0) then compute
safeAttendanceRate = Math.min(100, Math.max(0, attendanceNum)) and absenceRate =
100 - safeAttendanceRate, and update the progress indicator div (the element
using safeAttendanceRate for width) to include role="progressbar",
aria-valuemin="0", aria-valuemax="100", aria-valuenow={safeAttendanceRate} and
either aria-label or aria-labelledby so screen readers can announce it; also use
safeAttendanceRate for any inline style width to avoid width: NaN% issues.
| default: 'flex-col gap-6 rounded-xl border py-6 shadow-sm', | ||
| onlyText: | ||
| 'cursor-pointer items-center justify-between hover:bg-container-neutral-interaction', | ||
| buttonSet: 'flex-col gap-500', |
There was a problem hiding this comment.
여러 스타일 클래스가 디자인 토큰 규칙과 불일치합니다.
gap-6, py-6, gap-2, px-6, text-sm, text-muted-foreground 같은 raw 유틸리티가 섞여 있어 토큰 기반 시스템과 어긋납니다. 이 파일은 공용 UI 컴포넌트라 토큰 기준으로 통일하는 게 좋습니다.
As per coding guidelines **/*.{ts,tsx}: Use typography design tokens: typo-h1~h3, typo-sub1~2, typo-body1~2, typo-caption1~2 and use spacing design tokens: p-100~500 for padding, gap-100~400 for gaps.
Also applies to: 115-116, 127-127, 137-137, 154-154, 161-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/card.tsx` around lines 13 - 16, The style variants
'default', 'onlyText', and 'buttonSet' in src/components/ui/card.tsx use raw
utility classes (e.g., gap-6, py-6, px-6, text-sm, text-muted-foreground);
replace these with the design tokens mandated by the project (spacing tokens
p-100…p-500 and gap-100…gap-400 for padding/gaps, and typography tokens
typo-h1…typo-caption2 for text) so the Card component uses token-based classes;
update all other occurrences mentioned (around the ranges referenced) to use the
corresponding token names instead of raw utilities while preserving the same
visual intent.
| <div | ||
| data-slot="card" | ||
| className={cn(cardVariants({ variant }), className)} | ||
| onClick={onClick} | ||
| role="button" | ||
| tabIndex={0} | ||
| {...props} |
There was a problem hiding this comment.
role="button"를 쓴 div에는 Enter/Space 키보드 활성화 처리가 필요합니다.
현재는 마우스 클릭만 동작하고 키보드 사용자 동작이 누락되어 접근성 이슈가 발생합니다.
⌨️ 제안 수정
<div
data-slot="card"
className={cn(cardVariants({ variant }), className)}
onClick={onClick}
role="button"
tabIndex={0}
+ onKeyDown={(e) => {
+ if (!onClick) return;
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ onClick();
+ }
+ }}
{...props}
>📝 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.
| <div | |
| data-slot="card" | |
| className={cn(cardVariants({ variant }), className)} | |
| onClick={onClick} | |
| role="button" | |
| tabIndex={0} | |
| {...props} | |
| <div | |
| data-slot="card" | |
| className={cn(cardVariants({ variant }), className)} | |
| onClick={onClick} | |
| role="button" | |
| tabIndex={0} | |
| onKeyDown={(e) => { | |
| if (!onClick) return; | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| onClick(); | |
| } | |
| }} | |
| {...props} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/card.tsx` around lines 55 - 61, The card div
(data-slot="card") uses role="button" but lacks keyboard activation; add an
onKeyDown handler in the Card component that listens for Enter (Enter/Return)
and Space keys and invokes the existing onClick prop (or onActivate) so keyboard
users can trigger the card; for Space call event.preventDefault() before
invoking onClick to avoid unwanted scrolling, and ensure the handler gracefully
handles undefined onClick and preserves other props spread onto the element.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/components/ui/CloseIcon.tsx (1)
15-15:⚠️ Potential issue | 🟡 Minor기본 클래스에서 비토큰 색상과
cursor-pointer를 제거해 주세요.Line 15의
text-neutral-800는 토큰 규칙을 위반하고, 기본cursor-pointer는 아이콘이 항상 클릭 가능한 것처럼 보이게 합니다. 커서는 버튼/클릭 래퍼에서 주는 편이 안전합니다.🎨 제안 수정
- className={cn('cursor-pointer text-neutral-800', className)} + className={cn('text-text-alternative', className)}As per coding guidelines
**/*.{ts,tsx}: Use design tokens for text colors:text-text-strong,text-text-normal,text-text-alternative,text-text-disabled,text-text-inverse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/CloseIcon.tsx` at line 15, In CloseIcon (the component using className={cn('cursor-pointer text-neutral-800', className)}), remove the hardcoded 'cursor-pointer' and 'text-neutral-800' tokens from the default class list so the icon does not force a pointer or a non-token color; leave only neutral/default classes (or none) and let callers/parent button wrappers provide cursor and a design-token color (e.g., text-text-*) via the className prop; update the className usage in CloseIcon accordingly so consumers can opt into pointer behavior and token-based text colors.src/components/ui/modal/ModalFooter.tsx (1)
25-33:⚠️ Potential issue | 🟡 Minor
gap-[10px]는 토큰으로 교체하고 footer 메타 렌더링을 분리해 주세요.Line [25]의 임의값 간격과 Line [27]~Line [33] 인라인 삼항식이 가독성과 규칙 일관성을 떨어뜨립니다.
🧹 제안 수정안
function ModalFooter({ children, description, pagination, showDivider = true, className, ...props }: ModalFooterProps) { + const footerMeta = + pagination ?? + (description ? ( + <p className="typo-caption2 text-text-alternative mt-200 text-center">{description}</p> + ) : null); + return ( <div className={cn('flex flex-col', className)} {...props}> {showDivider && <Divider />} - <div className="flex flex-col gap-[10px] p-400"> + <div className="flex flex-col gap-100 p-400"> {children} - {pagination - ? pagination - : description && ( - <p className="typo-caption2 text-text-alternative mt-200 text-center"> - {description} - </p> - )} + {footerMeta} </div> </div> ); }As per coding guidelines "Use spacing design tokens:
p-100~500for padding,gap-100~400for gaps".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/modal/ModalFooter.tsx` around lines 25 - 33, Replace the hardcoded gap-[10px] token in the ModalFooter JSX with the appropriate spacing design token (e.g., gap-100..gap-400) and refactor the inline ternary that chooses between pagination and description into a separate render block or helper inside the ModalFooter component (use the existing children, pagination, and description props) so the footer meta rendering is clearer and consistent with spacing/token guidelines; locate the JSX that contains className="flex flex-col gap-[10px] p-400" and the inline ternary using pagination ? pagination : description && (...) and update them accordingly.src/app/(public)/attendance/page.tsx (1)
157-157:⚠️ Potential issue | 🟡 Minor온보딩 모달을 외부에서 닫을 때
currentPage초기화가 누락됩니다.Line [157]의
onOpenChange={setOnboardingModalOpen}는 열림 상태만 갱신해서, 닫힌 뒤 재오픈 시 이전 페이지가 유지될 수 있습니다. Line [199]~Line [200]처럼 닫힘 시setCurrentPage(1)을 같이 보장해 주세요.🧩 제안 수정안
- <Modal open={onboardingModalOpen} onOpenChange={setOnboardingModalOpen}> + <Modal + open={onboardingModalOpen} + onOpenChange={(open) => { + setOnboardingModalOpen(open); + if (!open) setCurrentPage(1); + }} + >Also applies to: 199-200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(public)/attendance/page.tsx at line 157, The Modal's onOpenChange currently only calls setOnboardingModalOpen, so when the modal is closed externally currentPage isn't reset and reopening preserves the previous page; update the onOpenChange handler for Modal (the one using onboardingModalOpen and setOnboardingModalOpen) to also call setCurrentPage(1) when the new open state is false (i.e., closed), ensuring currentPage is reset on close; apply the same change to the other place where setOnboardingModalOpen is used so both handlers reset currentPage via setCurrentPage(1) when closing.src/components/ui/modal/Modal.tsx (1)
50-56:⚠️ Potential issue | 🟠 Major모달 기본 접근성(role/aria/키보드 처리)이 필요합니다.
Line [50]~Line [56]의 컨테이너에
role="dialog"/aria-modal="true"/라벨 연결이 없고, Escape 닫기 및 포커스 복귀 처리도 빠져 있어 접근성 완성도가 떨어집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/modal/Modal.tsx` around lines 50 - 56, The Modal component's root/modal container is missing proper ARIA and keyboard handling: add role="dialog" and aria-modal="true" to the modal content div and connect a label/description via aria-labelledby/aria-describedby (use the modal title element id and optional description id), make the modal container focusable (e.g., tabIndex), implement Escape key handling to call handleClose (use a keydown listener in useEffect tied to the Modal component), implement simple focus management by saving document.activeElement before open, moving focus into the modal (e.g., to the close button or first focusable child) and restoring focus on close, and optionally add a basic focus trap to keep focus inside the modal while open; reference the Modal component, the modal content div, handleClose, and children when making these changes.
🧹 Nitpick comments (2)
src/components/ui/modal/ModalFooter.tsx (1)
7-23:ModalFooter도 UI 공통 패턴(cva/VariantProps/asChild) 적용을 권장합니다.Line [7]~Line [23]은 동작은 충분하지만,
src/components/ui공통 확장 패턴(variants + Slot 합성)이 빠져 재사용 시 확장성이 제한됩니다.As per coding guidelines "Component pattern: Define variants using cva, extend React.HTMLAttributes and VariantProps for Props interface, expose className prop, and support asChild for Radix components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/modal/ModalFooter.tsx` around lines 7 - 23, Update ModalFooter to follow the ui component pattern: define variants with cva (e.g., modalFooterVariants) and add VariantProps to ModalFooterProps, support an asChild prop and render via Slot when asChild is true, keep existing props (children, description, pagination, showDivider) and expose className while merging with cn and variant classes; update the component signature (ModalFooterProps and function ModalFooter) to extend React.HTMLAttributes<HTMLDivElement> and VariantProps<typeof modalFooterVariants>, add asChild?: boolean, apply variant classes from modalFooterVariants when computing className, and render either Slot or div based on asChild.src/components/ui/Divider.tsx (1)
19-23:Divider도 공통 UI 패턴(asChild) 확장 포인트를 맞춰주세요.현재 구현은 잘 동작하지만, Line [19]~Line [23]에서
asChild확장이 없어src/components/ui공통 패턴과 일관성이 깨집니다.♻️ 제안 수정안
+import { Slot } from '@radix-ui/react-slot'; import * as React from 'react'; import { cva, type VariantProps } from 'class-variance-authority'; import { cn } from '@/lib/cn'; interface DividerProps - extends React.HTMLAttributes<HTMLHRElement>, VariantProps<typeof dividerVariants> {} + extends React.HTMLAttributes<HTMLHRElement>, VariantProps<typeof dividerVariants> { + asChild?: boolean; +} -function Divider({ className, orientation, ...props }: DividerProps) { - return <hr className={cn(dividerVariants({ orientation }), className)} {...props} />; +function Divider({ className, orientation, asChild = false, ...props }: DividerProps) { + const Comp = asChild ? Slot : 'hr'; + return <Comp className={cn(dividerVariants({ orientation }), className)} {...props} />; }As per coding guidelines "Component pattern: Define variants using cva, extend React.HTMLAttributes and VariantProps for Props interface, expose className prop, and support asChild for Radix components".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/Divider.tsx` around lines 19 - 23, The Divider component needs to support the common "asChild" extension: add an optional asChild?: boolean to DividerProps (keep extending React.HTMLAttributes<HTMLHRElement> and VariantProps<typeof dividerVariants>), import Slot from "@radix-ui/react-slot", and in the Divider function render a Slot when asChild is true and an hr when not—both receiving the computed className (cn(dividerVariants({ orientation }), className)) and spreading ...props so consumers can replace the underlying element while preserving styles; ensure the prop type and render logic reference Divider, DividerProps, dividerVariants, and the Slot import.
🤖 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/app/`(public)/attendance/page.tsx:
- Line 19: The root div's className uses a raw Tailwind gap utility "gap-6"
which violates the spacing token rule; update the className on the JSX element
that currently reads "mt-300 flex flex-col gap-6 px-450" to use a design-token
gap (one of gap-100..gap-400) instead (e.g., replace "gap-6" with "gap-200" or
whichever token matches the intended spacing), keeping the other tokens (mt-300,
px-450) unchanged.
In `@src/components/ui/Divider.tsx`:
- Around line 22-23: In Divider, when the prop orientation === "vertical" add
the accessibility attribute aria-orientation="vertical" to the rendered <hr> so
assistive tech receives the correct context; update the Divider component
(function Divider) to conditionally include aria-orientation="vertical"
alongside the existing className computed by dividerVariants({ orientation })
and preserve other {...props} as before.
In `@src/components/ui/modal/Modal.tsx`:
- Around line 23-29: The handleClose callback currently calls close() when
isControlled is true but onOpenChange is not provided, which mutates the global
store in controlled mode; change the logic in handleClose so that when
isControlled is true you only attempt onOpenChange?.(false) and return (do not
call close()), otherwise call close(); update the handleClose body accordingly
(refer to handleClose, isControlled, onOpenChange, and close).
- Line 52: The overlay div in Modal (the element with onClick={handleClose})
uses raw Tailwind color class `bg-neutral-800/60`; replace it with the project's
design-token class (e.g., `bg-container-neutral-interaction` or the project's
overlay token utility) so the background color follows token naming
(`bg-container-neutral`, `bg-container-neutral-interaction`, etc.); update the
className on that div in Modal.tsx to use the appropriate token-based class and
remove the hardcoded `bg-neutral-800/60`.
---
Duplicate comments:
In `@src/app/`(public)/attendance/page.tsx:
- Line 157: The Modal's onOpenChange currently only calls
setOnboardingModalOpen, so when the modal is closed externally currentPage isn't
reset and reopening preserves the previous page; update the onOpenChange handler
for Modal (the one using onboardingModalOpen and setOnboardingModalOpen) to also
call setCurrentPage(1) when the new open state is false (i.e., closed), ensuring
currentPage is reset on close; apply the same change to the other place where
setOnboardingModalOpen is used so both handlers reset currentPage via
setCurrentPage(1) when closing.
In `@src/components/ui/CloseIcon.tsx`:
- Line 15: In CloseIcon (the component using className={cn('cursor-pointer
text-neutral-800', className)}), remove the hardcoded 'cursor-pointer' and
'text-neutral-800' tokens from the default class list so the icon does not force
a pointer or a non-token color; leave only neutral/default classes (or none) and
let callers/parent button wrappers provide cursor and a design-token color
(e.g., text-text-*) via the className prop; update the className usage in
CloseIcon accordingly so consumers can opt into pointer behavior and token-based
text colors.
In `@src/components/ui/modal/Modal.tsx`:
- Around line 50-56: The Modal component's root/modal container is missing
proper ARIA and keyboard handling: add role="dialog" and aria-modal="true" to
the modal content div and connect a label/description via
aria-labelledby/aria-describedby (use the modal title element id and optional
description id), make the modal container focusable (e.g., tabIndex), implement
Escape key handling to call handleClose (use a keydown listener in useEffect
tied to the Modal component), implement simple focus management by saving
document.activeElement before open, moving focus into the modal (e.g., to the
close button or first focusable child) and restoring focus on close, and
optionally add a basic focus trap to keep focus inside the modal while open;
reference the Modal component, the modal content div, handleClose, and children
when making these changes.
In `@src/components/ui/modal/ModalFooter.tsx`:
- Around line 25-33: Replace the hardcoded gap-[10px] token in the ModalFooter
JSX with the appropriate spacing design token (e.g., gap-100..gap-400) and
refactor the inline ternary that chooses between pagination and description into
a separate render block or helper inside the ModalFooter component (use the
existing children, pagination, and description props) so the footer meta
rendering is clearer and consistent with spacing/token guidelines; locate the
JSX that contains className="flex flex-col gap-[10px] p-400" and the inline
ternary using pagination ? pagination : description && (...) and update them
accordingly.
---
Nitpick comments:
In `@src/components/ui/Divider.tsx`:
- Around line 19-23: The Divider component needs to support the common "asChild"
extension: add an optional asChild?: boolean to DividerProps (keep extending
React.HTMLAttributes<HTMLHRElement> and VariantProps<typeof dividerVariants>),
import Slot from "@radix-ui/react-slot", and in the Divider function render a
Slot when asChild is true and an hr when not—both receiving the computed
className (cn(dividerVariants({ orientation }), className)) and spreading
...props so consumers can replace the underlying element while preserving
styles; ensure the prop type and render logic reference Divider, DividerProps,
dividerVariants, and the Slot import.
In `@src/components/ui/modal/ModalFooter.tsx`:
- Around line 7-23: Update ModalFooter to follow the ui component pattern:
define variants with cva (e.g., modalFooterVariants) and add VariantProps to
ModalFooterProps, support an asChild prop and render via Slot when asChild is
true, keep existing props (children, description, pagination, showDivider) and
expose className while merging with cn and variant classes; update the component
signature (ModalFooterProps and function ModalFooter) to extend
React.HTMLAttributes<HTMLDivElement> and VariantProps<typeof
modalFooterVariants>, add asChild?: boolean, apply variant classes from
modalFooterVariants when computing className, and render either Slot or div
based on asChild.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/app/(public)/attendance/page.tsxsrc/components/ui/CloseIcon.tsxsrc/components/ui/Divider.tsxsrc/components/ui/modal/Modal.tsxsrc/components/ui/modal/ModalBody.tsxsrc/components/ui/modal/ModalFooter.tsx
| const [currentPage, setCurrentPage] = useState(1); | ||
|
|
||
| return ( | ||
| <div className="mt-300 flex flex-col gap-6 px-450"> |
There was a problem hiding this comment.
페이지 루트 gap 값도 spacing 토큰으로 통일해 주세요.
Line [19]의 gap-6은 토큰 기반 간격 규칙과 불일치합니다 (gap-100~400 범위 사용 권장).
As per coding guidelines "Use spacing design tokens: p-100~500 for padding, gap-100~400 for gaps".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`(public)/attendance/page.tsx at line 19, The root div's className
uses a raw Tailwind gap utility "gap-6" which violates the spacing token rule;
update the className on the JSX element that currently reads "mt-300 flex
flex-col gap-6 px-450" to use a design-token gap (one of gap-100..gap-400)
instead (e.g., replace "gap-6" with "gap-200" or whichever token matches the
intended spacing), keeping the other tokens (mt-300, px-450) unchanged.
There was a problem hiding this comment.
제가 전에 모달 프로바이더 만들겠다 얘기 하고 만들지 않았었는데, 그 이유가 shadcn에서 alert dialog랑 dialog 다운 받아서 디자인만 수정해 쓰면 될 것 같아서 그랫습니다,,, 다현님이 이번 pr에서 구현하신 modal을 사용할 거면 모달 프로바이더 만들어서 페이지마다 useState로 모달 관리하지 않도록 설정하는 게 좋을 거 같아요!
개인적으로는,, dialog 수정해서 쓰는게 좋을 거 같긴 한데 이미 다 구현 해 놓으셔서,, 다른 분들의 의견도 궁금합니다,,,
아 그리구 svg 색 변경 하려면
<span
aria-label="Weeth"
className="bg-text-alternative block h-10 w-10 shrink-0"
style={{
maskImage: `url(${(logoIcon as StaticImageData).src})`,
WebkitMaskImage: `url(${(logoIcon as StaticImageData).src})`,
maskRepeat: 'no-repeat',
maskPosition: 'center',
maskSize: 'contain',
}}
/>
요런식으로 하면 되지 않을까,,, 싶어요 헤더에 위드 로고 색 변경을 햇거든요 자세히 보시려면 /components/admin/layout/LNB 참고해 주세요!
src/app/(public)/attendance/page.tsx
Outdated
|
|
||
| export default function AttendancePage() { | ||
| const { open, close } = useModalStore(); | ||
| const [simpleModalOpen, setSimpleModalOpen] = useState(false); |
There was a problem hiding this comment.
이렇게 되면 모달 쓰는 페이지는 다 use client로 해야 되네요.. 흠.. 페이지 전체를 use client로 하기보다 하위 컴포넌트 단위로 use client 하는 게 더 나을 거 같은데 어떻게 하는 게 좋을지 생각을 해봐야겟어요
src/components/ui/modal/Modal.tsx
Outdated
There was a problem hiding this comment.
근데 hoxy... ui에 shadcn으로 dialog 받아놓은 거 수정 안 하고 직접 구현하신 이유가 잇을까요..!?!? 수정하는 게 더 힘들어서..?? 왜냐면 dialog는
<DialogTrigger asChild>
<Button variant="outline">Share</Button>
</DialogTrigger>
이렇게 트리거가 있어서 useState로 모달 트리거를 안 해도 되니까.. 이거 디자인만 수정하는 게 더 편할 거라 생각 햇거등요..
There was a problem hiding this comment.
아하 이렇게 shadcn에서 트리거가 있어서 useState를 안 쓰는 방향이라면 프로바이더 대신 dialog를 수정하는 방향으로 가도 좋을 듯 하네요!
There was a problem hiding this comment.
요것두 shadcn 안 쓰고 직접 구현하신 이유가 잇으신가요??
There was a problem hiding this comment.
shadcn 쓸까 고민을 했는데, Shadcn의 Progress 컴포넌트는 보통 어떤 task의 진행률을 보여주기 위한 용도에 최적화되어 있어서 상태 변화에 따른 애니메이션과 함께 동적으로 진행 상황을 표시할 때 적합하다고 생각했습니다!
근데 출석률 progress bar는 서버에서 받아온 고정된 값(attendanceRate)을 단순 시각화하는 용도이고 굳이 클라이언트 컴포넌트로 만들어서 애니메이션 로직을 포함할 필요가 없다고 판단했습니다...!
단순한 스타일링으로 구현하는 것이 더 가볍고 직관적이라 생각하여 직접 구현한 것 입니다,, !
저도 유진님 의견이랑 비슷합니다! 매 페이지마다 그래서 개인적으로는 한 곳에서 열고 닫는 로직을 관리하면 재사용성쪽으로도 좋아지고 페이지 단에서는 단순히 다현님이 구현하신 부분이라서 프로바이더랑 shadcn dialog 중에서 한 번 더 생각해보시면 좋을 것 같애욤!! |
src/components/ui/modal/Modal.tsx
Outdated
There was a problem hiding this comment.
아하 이렇게 shadcn에서 트리거가 있어서 useState를 안 쓰는 방향이라면 프로바이더 대신 dialog를 수정하는 방향으로 가도 좋을 듯 하네요!
src/components/ui/modal/Modal.tsx
Outdated
| } | ||
|
|
||
| function Modal({ open: controlledOpen, onOpenChange, children }: ModalProps) { | ||
| const [mounted, setMounted] = React.useState(false); |
There was a problem hiding this comment.
React.useState 보다는 useState named import로 사용하는 게 현재 Next 컨벤션이라 통일하면 좋을 것 같습니다!
표준은 거의 아래와 같은 형태라고 해서...! 한 번 확인부탁드립니다!
import { useState } from 'react';
const [mounted, setMounted] = useState(false);
There was a problem hiding this comment.
넵 수정하겠습니다 ! 이런 것도,, 클로드한테 알려주는 게 좋을 것 같기도 하네요..
There was a problem hiding this comment.
shadcn dialog 수정하는 방향으로 해서 해당 파일은 삭제했습니다!
src/components/ui/modal/Modal.tsx
Outdated
| React.useEffect(() => { | ||
| setMounted(true); | ||
| return () => setMounted(false); | ||
| }, []); | ||
|
|
||
| React.useEffect(() => { | ||
| if (isOpen) { | ||
| document.body.style.overflow = 'hidden'; | ||
| } else { | ||
| document.body.style.overflow = ''; | ||
| } | ||
| return () => { | ||
| document.body.style.overflow = ''; | ||
| }; | ||
| }, [isOpen]); |
There was a problem hiding this comment.
요 부분도 React.useEffect 대신 다른 hooks처럼 useEffect named import로 통일하면 좋을 것 같아요!
overflow 설정 로직이 if/else + cleanup으로 조금 길어 보이는데,
document.body.style.overflow = isOpen ? 'hidden' : ''
형태로 줄이면 중복도 줄고 의도가 더 바로 보일 것 같아요! 추천 전체 코드는 아래와 같습니당!! 다들 오뜨케 생각하시나용?
import { useEffect } from 'react';
useEffect(() => {
setMounted(true);
return () => setMounted(false);
}, []);
useEffect(() => {
document.body.style.overflow = isOpen ? 'hidden' : '';
return () => {
document.body.style.overflow = '';
};
}, [isOpen]);
There was a problem hiding this comment.
이것두,, 파일 삭제돼서 반영하지 않아도 될 것 같습니다! 의견 감사합니다 ~
src/components/ui/ArrowRightIcon.tsx
Outdated
There was a problem hiding this comment.
저도 유진님 말씀처럼 svg를 mask로 처리하거나 currentColor 기반으로 색을 제어하는 방식이 더 좋을 것 같습니당!!
There was a problem hiding this comment.
currentColor로 색 제어하는 방식도 해봤는데 라이트/다크 모드에 따라 컬러 제어가 안돼서 한번 유진님이 알려준 방식대로 해보겠습니다,,
nabbang6
left a comment
nabbang6
left a comment
There was a problem hiding this comment.
저도 내부 구현은 shadcn Dialog 기반으로 수정하고, Provider로 래핑하는 방식 좋은 것 같습니다! 모달이 단순한 confirm 수준이라면 트리거 방식만으로 관리해도 충분할 것 같지만,, 현재 페이지네이션이나 온보딩 모달처럼 다양한 형태가 있어서 Provider로 공통적으로 관리하는 것도 좋을 것 같습니당,, 재사용성이나 확장성 측면에서도 그쪽이 조금 더 유연할 것 같아요!
그리고 만약 현재 구현 그대로 사용하게 된다면,, 열고 닫을 때 애니메이션이 빠져 있어서 조오금 끊기는 느낌이 있어서,, 간단하게 fade나 slide 같은 전환 효과도 추가해주심 좋을 것 같습니당! b
고생하셨어요~!! 👍👍
src/components/ui/modal/Modal.tsx
Outdated
There was a problem hiding this comment.
현재 z-index 기준으로 오버레이와 모달 컨텐츠가 분리돼 있어서 문제는 없지만, 추후 구조가 변경될 경우 모달 내부 클릭이 오버레이까지 버블링될 수도 있을 것 같습니다! 모달 컨텐츠 div에 onClick={(e) => e.stopPropagation()} 넣어두면 조금 더 안전하게 관리할 수 있을 것 같아욥 b
There was a problem hiding this comment.
기존 dialog 모달 수정하는 방향으로 하게 되어서 해당 Modal 파일은 삭제했습니다~!! 의견 감사합니당
페이지마다 useState로 관리하는 건 아니고 zustand로 전역 상태 관리하도록 되어 있긴 한데, 여러 모달을 동시에 사용하기는 어려워서 예시로 모달 여러 개 보여줄 때 useState를 사용한 것 같습니다. |
…into feat/WTH-171-출석-페이지-컴포넌트-구현
PR 검증 결과✅ TypeScript: 통과 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/(public)/attendance/page.tsx (2)
23-23:⚠️ Potential issue | 🟡 Minor
gap-6을 spacing 디자인 토큰으로 변경해 주세요.
gap-6은 디자인 토큰 규칙(gap-100~400)과 일치하지 않습니다. As per coding guidelines: "Use spacing design tokens:p-100~500for padding,gap-100~400for gaps".🔧 제안 수정안
- <div className="mt-300 flex flex-col gap-6 px-450"> + <div className="mt-300 flex flex-col gap-400 px-450">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(public)/attendance/page.tsx at line 23, Replace the non-token spacing class in the element with className="mt-300 flex flex-col gap-6 px-450": change gap-6 to a spacing design token in the allowed range (e.g., gap-300) so the class becomes gap-300, keeping mt-300 and px-450 unchanged; update the JSX in the page component where that className appears.
176-235:⚠️ Potential issue | 🟡 Minor온보딩 모달을 외부에서 닫을 때
currentPage가 초기화되지 않습니다.이 Dialog는 uncontrolled 모드로 동작하여
onOpenChange를 사용하지 않습니다. 사용자가 오버레이나 ESC 키로 모달을 닫으면currentPage가 유지되어 재오픈 시 중간 페이지에서 시작합니다. controlled 모드로 전환하여 닫힐 때 상태를 초기화하는 것을 권장합니다.🔧 제안 수정안 - controlled 모드로 전환
export default function AttendancePage() { const [currentPage, setCurrentPage] = useState(1); + const [onboardingOpen, setOnboardingOpen] = useState(false); // ... - <Dialog> + <Dialog + open={onboardingOpen} + onOpenChange={(open) => { + setOnboardingOpen(open); + if (!open) setCurrentPage(1); + }} + > <DialogTrigger asChild>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`(public)/attendance/page.tsx around lines 176 - 235, The Dialog is uncontrolled so closing via overlay/ESC preserves currentPage; make it controlled by adding an isOpen state (useState) and pass open={isOpen} and onOpenChange={(open) => { setIsOpen(open); if (!open) setCurrentPage(1); }} to the Dialog (referencing Dialog, DialogTrigger, DialogContent, DialogFooter, DialogClose, currentPage, setCurrentPage, and setIsOpen) so any close action resets currentPage to 1; ensure DialogClose and any "다음" button update isOpen appropriately (e.g., setIsOpen(false) when finishing) so the modal fully closes and resets.
🧹 Nitpick comments (2)
src/components/ui/dialog.tsx (2)
78-85:onClose타입이 CloseIcon의onClick핸들러와 정확히 일치하지 않습니다.
onClose: () => void로 정의되어 있지만,CloseIcon의onClick은MouseEventHandler<SVGSVGElement>를 기대합니다. 현재 React의 유연한 타입 처리 덕분에 동작하지만, 명시적 타입 안전성을 위해 이벤트 파라미터를 추가하거나 래퍼 함수를 사용하는 것이 좋습니다.♻️ 제안 수정안
interface DialogHeaderProps extends React.HTMLAttributes<HTMLDivElement> { overline?: string; title?: string; description?: string; showClose?: boolean; - onClose?: () => void; + onClose?: (event?: React.MouseEvent) => void; children?: React.ReactNode; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/dialog.tsx` around lines 78 - 85, The onClose prop in DialogHeaderProps should match the CloseIcon onClick signature; update DialogHeaderProps to type onClose as a React.MouseEventHandler<SVGSVGElement> (or (event?: React.MouseEvent<SVGSVGElement>) => void) so it accepts the event parameter, and then adjust any call sites that pass a no-arg function by wrapping them (e.g., () => onClose?.(event) or event => onClose?.(event)) to preserve behavior; target the DialogHeaderProps interface and the CloseIcon onClick usage to ensure signature alignment.
98-111: 커스텀 children 모드에서 접근성 고려가 필요합니다.
children을DialogPrimitive.Title내부에 래핑하면, children이 복잡한 JSX(예: 아이콘 + 텍스트)인 경우 스크린 리더가 전체 내용을 타이틀로 읽게 됩니다. 필요시aria-label또는aria-labelledby를 추가할 수 있도록 props를 확장하는 것을 고려해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/dialog.tsx` around lines 98 - 111, The current custom-children branch wraps arbitrary children inside DialogPrimitive.Title which can cause screen readers to treat complex JSX as the dialog title; modify the component to accept and pass accessibility props (e.g., titleAriaLabel, titleAriaLabelledBy or aria-label/aria-labelledby from props) and apply them to the DialogPrimitive.Title (or the enclosing div) when rendering children so consumers can provide an explicit label or labelledby id; update the prop typing and ensure DialogPrimitive.Title receives the aria attributes while keeping existing behavior for showClose, onClose, className, and children.
🤖 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/app/`(public)/attendance/page.tsx:
- Around line 218-232: The DialogClose wrapper causes the dialog to always close
after the Button's onClick runs, so change to conditional close behavior: remove
DialogClose around the Button and instead handle closing inside the onClick
based on currentPage (use setCurrentPage to advance and only call the dialog's
close function or toggle the controlled open state when currentPage === 4), or
alternatively render DialogClose only when currentPage === 4; update the
Button's onClick to call setCurrentPage((p) => p < 4 ? p+1 : 1) and then perform
the dialog close action conditionally (use your dialog's close method or
controlled open prop) so the dialog remains open while paging and only closes on
the final step.
---
Duplicate comments:
In `@src/app/`(public)/attendance/page.tsx:
- Line 23: Replace the non-token spacing class in the element with
className="mt-300 flex flex-col gap-6 px-450": change gap-6 to a spacing design
token in the allowed range (e.g., gap-300) so the class becomes gap-300, keeping
mt-300 and px-450 unchanged; update the JSX in the page component where that
className appears.
- Around line 176-235: The Dialog is uncontrolled so closing via overlay/ESC
preserves currentPage; make it controlled by adding an isOpen state (useState)
and pass open={isOpen} and onOpenChange={(open) => { setIsOpen(open); if (!open)
setCurrentPage(1); }} to the Dialog (referencing Dialog, DialogTrigger,
DialogContent, DialogFooter, DialogClose, currentPage, setCurrentPage, and
setIsOpen) so any close action resets currentPage to 1; ensure DialogClose and
any "다음" button update isOpen appropriately (e.g., setIsOpen(false) when
finishing) so the modal fully closes and resets.
---
Nitpick comments:
In `@src/components/ui/dialog.tsx`:
- Around line 78-85: The onClose prop in DialogHeaderProps should match the
CloseIcon onClick signature; update DialogHeaderProps to type onClose as a
React.MouseEventHandler<SVGSVGElement> (or (event?:
React.MouseEvent<SVGSVGElement>) => void) so it accepts the event parameter, and
then adjust any call sites that pass a no-arg function by wrapping them (e.g.,
() => onClose?.(event) or event => onClose?.(event)) to preserve behavior;
target the DialogHeaderProps interface and the CloseIcon onClick usage to ensure
signature alignment.
- Around line 98-111: The current custom-children branch wraps arbitrary
children inside DialogPrimitive.Title which can cause screen readers to treat
complex JSX as the dialog title; modify the component to accept and pass
accessibility props (e.g., titleAriaLabel, titleAriaLabelledBy or
aria-label/aria-labelledby from props) and apply them to the
DialogPrimitive.Title (or the enclosing div) when rendering children so
consumers can provide an explicit label or labelledby id; update the prop typing
and ensure DialogPrimitive.Title receives the aria attributes while keeping
existing behavior for showClose, onClose, className, and children.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aaeb25d7-1d5d-4f24-992e-4457c63113c9
⛔ Files ignored due to path filters (1)
src/assets/icons/arrow_right.svgis excluded by!**/*.svg
📒 Files selected for processing (4)
src/app/(public)/attendance/page.tsxsrc/components/ui/dialog.tsxsrc/components/ui/index.tssrc/stores/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/stores/index.ts
- src/components/ui/index.ts
…into feat/WTH-171-출석-페이지-컴포넌트-구현
PR 검증 결과✅ TypeScript: 통과 🎉 모든 검증을 통과했습니다! |
|
구현한 기능 Preview: https://weeth-60e4h83nh-weethsite-4975s-projects.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/ui/card.tsx (2)
56-62:⚠️ Potential issue | 🟠 Major
role="button"요소에 키보드 활성화 처리가 빠져 있습니다.현재는 Enter/Space로 활성화할 수 없어 접근성 결함이 발생합니다.
onClick이 있을 때만 버튼 역할/포커스를 주고,onKeyDown으로 Enter/Space를 처리해 주세요.⌨️ 제안 수정
<div data-slot="card" className={cn(cardVariants({ variant }), className)} onClick={onClick} - role="button" - tabIndex={0} + role={onClick ? 'button' : undefined} + tabIndex={onClick ? 0 : undefined} + onKeyDown={(e) => { + if (!onClick) return; + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onClick(); + } + }} {...props} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/card.tsx` around lines 56 - 62, The card element currently sets role="button" and tabIndex=0 but doesn't handle keyboard activation; update the Card component (the element using cn(cardVariants({ variant }), className) with onClick) so role="button" and tabIndex are only applied when an onClick prop exists, and add an onKeyDown handler that triggers the provided onClick when Enter or Space is pressed (prevent default for Space). Ensure you reference the existing onClick prop and preserve other {...props} while wiring the new onKeyDown conditional behavior.
14-17:⚠️ Potential issue | 🟡 Minor디자인 토큰 규칙과 다른 raw 유틸리티 클래스가 남아 있습니다.
gap-6,py-6,gap-2,px-6,text-sm,text-muted-foreground등은 토큰 기반 시스템으로 치환해 주세요.As per coding guidelines
**/*.{ts,tsx}: Use typography design tokenstypo-h1~h3,typo-sub1~2,typo-body1~2,typo-caption1~2,typo-button1~2and spacing design tokensp-100~500,gap-100~400.Also applies to: 140-140, 162-162, 179-179, 186-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/card.tsx` around lines 14 - 17, The Card component's variant class strings (e.g., the "default", "onlyText", and "buttonSet" keys in src/components/ui/card.tsx) still use raw utility classes like gap-6, py-6, px-6, text-sm, text-muted-foreground; replace these with the design-token utilities: use gap-100~400 tokens (e.g., gap-300 instead of gap-6), padding tokens p-100~500 for py/px (e.g., p-300 or py-300), and typography tokens (typo-body1~2, typo-caption1~2, typo-button1~2) instead of text-sm/text-muted-foreground; update all occurrences in the Card variants and the other referenced locations (lines noted in the comment) so all spacing and typography classes follow the token naming scheme.
🧹 Nitpick comments (1)
src/components/ui/card.tsx (1)
11-25: Card의 variants/Props 타입도 모듈 API로 같이 노출하는 편이 좋습니다.공용 UI 컴포넌트 확장성을 위해
cardVariants와CardProps를 export해 두면src/components/ui/index.ts에서 일관된 배럴 공개가 가능합니다.As per coding guidelines
src/components/ui/index.ts: Export new UI components and their variants/Props types fromsrc/components/ui/index.ts.Also applies to: 192-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/card.tsx` around lines 11 - 25, Export the cardVariants constant and the CardProps interface so they are publicly available (add exports for cardVariants and CardProps in the card component file) and then update the UI barrel (src/components/ui/index.ts) to re-export them alongside the Card component; locate the symbols cardVariants and CardProps in the Card component (the cva declaration and the interface line) and ensure they are exported from that module and included in the barrel's export list for consistent public API exposure.
🤖 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/ui/dialog.tsx`:
- Around line 119-130: The close control is implemented as a span with
aria-hidden and an onClick handler which prevents keyboard/assistive access;
replace the span with an actual interactive element (e.g., a <button> or use
DialogPrimitive.Close asChild) so the onClose handler is reachable via keyboard
and screen readers, remove aria-hidden, preserve the visual mask using the
deleteIcon StaticImageData (keep the maskImage / WebkitMaskImage style logic)
and apply the same change for the other instance using the same pattern around
lines containing onClose and deleteIcon.
- Around line 71-74: Replace the hardcoded utility class "bg-neutral-800" used
on the icon element (the element that sets maskImage/WebkitMaskImage using
deleteIcon as StaticImageData) with the approved design token background class
(choose the appropriate one from bg-container-neutral,
bg-container-neutral-interaction, bg-button-primary, or bg-button-neutral per
context); update the same change for the other two occurrences referenced (the
elements at the locations you flagged around lines 122-123 and 157-158) so all
icon background classes use the project design-token classes instead of
bg-neutral-800.
---
Duplicate comments:
In `@src/components/ui/card.tsx`:
- Around line 56-62: The card element currently sets role="button" and
tabIndex=0 but doesn't handle keyboard activation; update the Card component
(the element using cn(cardVariants({ variant }), className) with onClick) so
role="button" and tabIndex are only applied when an onClick prop exists, and add
an onKeyDown handler that triggers the provided onClick when Enter or Space is
pressed (prevent default for Space). Ensure you reference the existing onClick
prop and preserve other {...props} while wiring the new onKeyDown conditional
behavior.
- Around line 14-17: The Card component's variant class strings (e.g., the
"default", "onlyText", and "buttonSet" keys in src/components/ui/card.tsx) still
use raw utility classes like gap-6, py-6, px-6, text-sm, text-muted-foreground;
replace these with the design-token utilities: use gap-100~400 tokens (e.g.,
gap-300 instead of gap-6), padding tokens p-100~500 for py/px (e.g., p-300 or
py-300), and typography tokens (typo-body1~2, typo-caption1~2, typo-button1~2)
instead of text-sm/text-muted-foreground; update all occurrences in the Card
variants and the other referenced locations (lines noted in the comment) so all
spacing and typography classes follow the token naming scheme.
---
Nitpick comments:
In `@src/components/ui/card.tsx`:
- Around line 11-25: Export the cardVariants constant and the CardProps
interface so they are publicly available (add exports for cardVariants and
CardProps in the card component file) and then update the UI barrel
(src/components/ui/index.ts) to re-export them alongside the Card component;
locate the symbols cardVariants and CardProps in the Card component (the cva
declaration and the interface line) and ensure they are exported from that
module and included in the barrel's export list for consistent public API
exposure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c353439b-b848-42ec-a6ee-a929d7389357
📒 Files selected for processing (4)
src/components/ui/Divider.tsxsrc/components/ui/card.tsxsrc/components/ui/dialog.tsxsrc/components/ui/index.ts
| <span | ||
| aria-hidden | ||
| onClick={onClose} | ||
| className="block h-6 w-6 shrink-0 cursor-pointer bg-neutral-800" | ||
| style={{ | ||
| maskImage: `url(${(deleteIcon as StaticImageData).src})`, | ||
| WebkitMaskImage: `url(${(deleteIcon as StaticImageData).src})`, | ||
| maskRepeat: 'no-repeat', | ||
| maskPosition: 'center', | ||
| maskSize: 'contain', | ||
| }} | ||
| /> |
There was a problem hiding this comment.
헤더 닫기 컨트롤이 접근성 기준을 충족하지 못합니다.
aria-hidden이 붙은 <span>에 onClick을 연결하면 키보드/보조기기 사용자에게 닫기 기능이 사실상 막힙니다. button 또는 DialogPrimitive.Close asChild로 바꿔 주세요.
♿ 제안 수정
- {showClose && onClose && (
- <span
- aria-hidden
- onClick={onClose}
- className="block h-6 w-6 shrink-0 cursor-pointer bg-neutral-800"
- style={{
- maskImage: `url(${(deleteIcon as StaticImageData).src})`,
- WebkitMaskImage: `url(${(deleteIcon as StaticImageData).src})`,
- maskRepeat: 'no-repeat',
- maskPosition: 'center',
- maskSize: 'contain',
- }}
- />
- )}
+ {showClose && (
+ <DialogPrimitive.Close asChild>
+ <button
+ type="button"
+ aria-label="닫기"
+ onClick={onClose}
+ className="block h-6 w-6 shrink-0 cursor-pointer bg-button-neutral"
+ style={{
+ maskImage: `url(${(deleteIcon as StaticImageData).src})`,
+ WebkitMaskImage: `url(${(deleteIcon as StaticImageData).src})`,
+ maskRepeat: 'no-repeat',
+ maskPosition: 'center',
+ maskSize: 'contain',
+ }}
+ />
+ </DialogPrimitive.Close>
+ )}Also applies to: 154-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/ui/dialog.tsx` around lines 119 - 130, The close control is
implemented as a span with aria-hidden and an onClick handler which prevents
keyboard/assistive access; replace the span with an actual interactive element
(e.g., a <button> or use DialogPrimitive.Close asChild) so the onClose handler
is reachable via keyboard and screen readers, remove aria-hidden, preserve the
visual mask using the deleteIcon StaticImageData (keep the maskImage /
WebkitMaskImage style logic) and apply the same change for the other instance
using the same pattern around lines containing onClose and deleteIcon.
nabbang6
left a comment
nabbang6
left a comment
There was a problem hiding this comment.
확인했습니다~!
현재 page.tsx 안에 여러 모달들이 함께 구현되어 있는데, 확정된 모달들(QR 출석 모달, 온보딩 모달 등...)은 별도 컴포넌트로 분리해도 좋을 것 같습니당
고생 많으셨어요 !!! 👍
| <Dialog> | ||
| <DialogTrigger asChild> | ||
| <Button variant="secondary" size="md"> | ||
| 온보딩 모달 | ||
| </Button> |
There was a problem hiding this comment.
현재 온보딩 모달이 닫혀도 currentPage 상태가 리셋되지 않고 있어서, 모달을 다시 열었을 때 이전 페이지 번호의 모달이 표시되는 문제가 있는 것 같아요! Dialog의 onOpenChange에서 모달이 닫힐 때 currentPage를 초기화해주면 좋을 것 같습니당
<Dialog onOpenChange={(open) => { if (!open) setCurrentPage(1); }}>
요런 느낌으로,,,
There was a problem hiding this comment.
이 페이지에 구현된 모달들은 ui 예시를 보여주기 위해 임시로 구현한거라,, 디테일한 사항들은 고려하지 않았습니다!
이 안에서 페이지네이션 같은 컴포넌트는 추후에 디자인 확정되면 해당 파트 맡은 분이 분리하고 나영님이 말씀해주신 상세한 작업들 진행주시면 좋을 것 같습니다..!
src/app/(public)/attendance/page.tsx
Outdated
| <Card | ||
| variant="buttonSet" | ||
| overline="오늘의 출석" | ||
| title="프론트엔드 중간 발표" | ||
| description="날짜: 2024년 7월 18일 (7:00 PM~8:30 PM)\n장소: 가천관 247호" | ||
| onSecondaryClick={() => console.log('출석코드 확인')} | ||
| /> |
There was a problem hiding this comment.
현재 버튼 클릭 시에 콘솔만 출력되고 있어서 하단에 구현해주신 출석 확인 모달 확인이 어려운 것 같습니다 ㅜ.^... 호옥시 버튼 클릭 > 모달 열리게 수정해주실 수 잇을까요...?
woneeeee
left a comment
woneeeee
left a comment
There was a problem hiding this comment.
수고하셨어요!! src/components/ui/dialog.tsx 파일 코멘트 반영해서 머지하면 좋을 듯 합니당!!
src/components/ui/dialog.tsx
Outdated
| return ( | ||
| <div | ||
| data-slot="dialog-header" | ||
| className={cn('flex items-center justify-between p-400', className)} |
There was a problem hiding this comment.
현재 모달이 제일 루트 모달 css에서 p-400이 적용되어있는데 헤더, 바디, 푸터에도 각각 패딩이 적용되어있는 것 같습니당!!
2026-03-07.3.56.39.mov
아예 모달 전역에서 p-400을 적용하고 헤더, 바디, 푸터에는 각각 py나 gap 상태를 넣어서 관리해주는건 어떨까욤??
There was a problem hiding this comment.
좋습니당 모달 전역 p-400만 좌우 padding 담당하고, 각 섹션은 상하 간격만 관리하도록 변경했습니다!
src/components/ui/card.tsx
Outdated
| {showArrow && ( | ||
| <span | ||
| aria-hidden | ||
| className="block h-4 w-4 shrink-0 bg-neutral-800" |
There was a problem hiding this comment.
여기에 cursor-pointer도 함께 넣어주면 좋을 듯 합니다!!
JIN921
left a comment
JIN921
left a comment
There was a problem hiding this comment.
리뷰 짱 길어졋네요... 많은 수정 사항 적용 하시느라 고생하셧습니다ㅜㅜ 이렇게 해놧으니까 페이지 만들 때 호다닥 만들 수 있겟어요🔥🔥
| pagination={ | ||
| <div className="flex items-center justify-center gap-300"> | ||
| <button | ||
| onClick={() => setCurrentPage((prev) => Math.max(1, prev - 1))} | ||
| className="typo-body2 text-text-alternative disabled:opacity-50" | ||
| disabled={currentPage === 1} | ||
| > | ||
| < | ||
| </button> | ||
| <span className="typo-body2 text-text-normal">{currentPage} / 4</span> | ||
| <button | ||
| onClick={() => setCurrentPage((prev) => Math.min(4, prev + 1))} | ||
| className="typo-body2 text-text-alternative disabled:opacity-50" | ||
| disabled={currentPage === 4} | ||
| > | ||
| > | ||
| </button> | ||
| </div> | ||
| } |
There was a problem hiding this comment.
요 페이지네이션 버튼 부분은 컴포넌트로 빼도 좋을 거 같애요 종종 쓰이지 않을까,, 하는 느낌,,, 나중에 추가될 팝업 공지에도 쓰이기 때문에,, 헤헤,, 추후에 버튼 저거만 svg로 바꾸고 페이지 props로 받아오게 해서 컴포넌트화 하면 좋을 거 같아요!
There was a problem hiding this comment.
arrow_right.svg는 이미 있어서 왼쪽 화살표만 svg 추가해두겠습니당
그거는 예시로 구현해둔거라,, 추후 UI 구현할 때 이 기본 모달 사용해서 각자 구현해주시면 될 것 같습니당 |
PR 검증 결과✅ TypeScript: 통과 🎉 모든 검증을 통과했습니다! |
|
구현한 기능 Preview: https://weeth-4yafw9c65-weethsite-4975s-projects.vercel.app |
PR 검증 결과✅ TypeScript: 통과 🎉 모든 검증을 통과했습니다! |
|
구현한 기능 Preview: https://weeth-oxeonv800-weethsite-4975s-projects.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/components/ui/card.tsx (1)
56-62:⚠️ Potential issue | 🟠 Major클릭 카드의 버튼 semantics가 반쪽입니다.
지금은
role="button"과tabIndex={0}만 있고 Enter/Space 처리가 없어 키보드로는 활성화할 수 없습니다. 게다가onClick이 없을 때도 포커스 가능한 버튼처럼 노출됩니다.onClick이 있을 때만 button semantics를 주고, Enter/Space 키도 함께 처리해 주세요.⌨️ 제안 수정
<div data-slot="card" - className={cn(cardVariants({ variant }), className)} + className={cn(cardVariants({ variant }), onClick && 'cursor-pointer', className)} onClick={onClick} - role="button" - tabIndex={0} + role={onClick ? 'button' : undefined} + tabIndex={onClick ? 0 : undefined} + onKeyDown={(e) => { + if (!onClick) return; + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + onClick(); + } + }} {...props} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/card.tsx` around lines 56 - 62, The clickable card currently always exposes button semantics but doesn't handle keyboard activation; update the element in card.tsx (the div with data-slot="card" and props onClick/className/cardVariants) so role="button" and tabIndex={0} are only applied when onClick is provided, and add an onKeyDown handler (e.g., handleCardKeyDown) that listens for Enter and Space keys to call onClick (call preventDefault for Space to avoid page scroll) and forwards events to onClick; ensure the existing onClick is still invoked for mouse clicks and that props are spread as before.src/components/ui/dialog.tsx (1)
118-130:⚠️ Potential issue | 🟠 Major닫기 아이콘은 실제 인터랙티브 엘리먼트여야 합니다.
aria-hidden인<span>에onClick을 붙이면 키보드/보조기기에서 닫기 동작에 접근할 수 없습니다. 이 두 곳은button또는DialogPrimitive.Close asChild로 바꾸고aria-label을 주는 편이 맞습니다.Also applies to: 153-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/dialog.tsx` around lines 118 - 130, The close icon is rendered as a non-interactive <span aria-hidden> with onClick, making it inaccessible; replace that span with an actual interactive element (either a <button> or use DialogPrimitive.Close asChild) where the icon is the child, remove aria-hidden, and add an appropriate aria-label (e.g., "Close") so keyboard and assistive tech can activate it; apply this change for the instances that check showClose and onClose (references: showClose, onClose, deleteIcon) and the analogous block later in the file (the second span at lines ~153-165).
🧹 Nitpick comments (1)
src/components/ui/dialog.tsx (1)
203-203: 여기서는 raw gap 대신 spacing token을 써 주세요.
gap-[10px]는 공용 UI 컴포넌트의 토큰 규칙과 맞지 않습니다.gap-100~400범위의 spacing token으로 맞추는 편이 좋겠습니다. As per coding guidelines**/*.{ts,tsx}: Use spacing design tokens:p-100~500for padding,gap-100~400for gaps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ui/dialog.tsx` at line 203, Replace the hardcoded utility "gap-[10px]" in the dialog component's JSX (the div with className containing "gap-[10px] pt-400") with the appropriate spacing design token from gap-100..gap-400 to conform to the shared UI token rules; pick the nearest semantic token (e.g., gap-100 or gap-200) that matches the intended spacing and update the className to use that token instead of the raw pixel value.
🤖 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/ui/dialog.tsx`:
- Around line 191-203: The DialogFooter component currently defaults showDivider
to true which introduces an unexpected Divider in existing usages; change the
default in the DialogFooter parameter list from showDivider = true to
showDivider = false so the divider is opt-in, and verify callers (e.g., places
that simply pass children like page.tsx) that need the divider explicitly pass
showDivider={true}; update any relevant tests or stories that assume the old
default.
- Around line 107-117: The current DialogHeader wraps the entire children prop
in DialogPrimitive.Title which causes nested DialogTitle/DialogDescription
passed as children to be treated all as a title; update the children-mode branch
in DialogHeader so that when children is provided it renders children directly
(no surrounding DialogPrimitive.Title), preserving the container div attributes
(data-slot, className, props) and leaving accessibility/title/description roles
to the caller components such as DialogTitle and DialogDescription.
---
Duplicate comments:
In `@src/components/ui/card.tsx`:
- Around line 56-62: The clickable card currently always exposes button
semantics but doesn't handle keyboard activation; update the element in card.tsx
(the div with data-slot="card" and props onClick/className/cardVariants) so
role="button" and tabIndex={0} are only applied when onClick is provided, and
add an onKeyDown handler (e.g., handleCardKeyDown) that listens for Enter and
Space keys to call onClick (call preventDefault for Space to avoid page scroll)
and forwards events to onClick; ensure the existing onClick is still invoked for
mouse clicks and that props are spread as before.
In `@src/components/ui/dialog.tsx`:
- Around line 118-130: The close icon is rendered as a non-interactive <span
aria-hidden> with onClick, making it inaccessible; replace that span with an
actual interactive element (either a <button> or use DialogPrimitive.Close
asChild) where the icon is the child, remove aria-hidden, and add an appropriate
aria-label (e.g., "Close") so keyboard and assistive tech can activate it; apply
this change for the instances that check showClose and onClose (references:
showClose, onClose, deleteIcon) and the analogous block later in the file (the
second span at lines ~153-165).
---
Nitpick comments:
In `@src/components/ui/dialog.tsx`:
- Line 203: Replace the hardcoded utility "gap-[10px]" in the dialog component's
JSX (the div with className containing "gap-[10px] pt-400") with the appropriate
spacing design token from gap-100..gap-400 to conform to the shared UI token
rules; pick the nearest semantic token (e.g., gap-100 or gap-200) that matches
the intended spacing and update the className to use that token instead of the
raw pixel value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7f3e8f7-71ca-490b-a194-32eb89e08418
⛔ Files ignored due to path filters (1)
src/assets/icons/arrow_left.svgis excluded by!**/*.svg
📒 Files selected for processing (3)
src/app/(public)/attendance/page.tsxsrc/components/ui/card.tsxsrc/components/ui/dialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/(public)/attendance/page.tsx
PR 검증 결과✅ TypeScript: 통과 🎉 모든 검증을 통과했습니다! |
|
구현한 기능 Preview: https://weeth-pe1171h4u-weethsite-4975s-projects.vercel.app |
4 participants
✅ PR 유형
어떤 변경 사항이 있었나요?
📌 관련 이슈번호
✅ Key Changes
📸 스크린샷 or 실행영상
/attendance 페이지 들어가서 확인해주시면 됩니당 !!
🎸 기타 사항 or 추가 코멘트
attendance/page.tsx예제 코드랑 ,dummy.svg는 추후에 삭제하겠습니당Summary by CodeRabbit
릴리스 노트