Conversation
Walkthrough타이머 페이지에 브라우저 전체화면 제어 훅과 UI(토글 버튼, 도움말 문구)가 추가되고, 관련 훅과 타입이 공개 API에 포함되었습니다. 헤더의 버튼/스타일과 여러 아이콘( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor 사용자 as User
participant TimerPage as TimerPage(UI)
participant Hook as useFullscreen / useTimerPageState
participant Browser as Document / Element
User->>TimerPage: 전체 화면 버튼 클릭
TimerPage->>Hook: toggleFullscreen()
alt 진입 (isFullscreen=false)
Hook->>Browser: element.requestFullscreen() (or vendor-prefixed)
Note right of Browser #DFF2E1: 브라우저가 fullscreen 진입\n-> fullscreenchange 이벤트 발생
else 해제 (isFullscreen=true)
Hook->>Browser: document.exitFullscreen() (or vendor-prefixed)
Note right of Browser #FEECE7: 브라우저가 fullscreen 해제\n-> fullscreenchange 이벤트 발생
end
Browser-->>Hook: fullscreenchange 이벤트
Hook->>Hook: isFullscreen 상태 업데이트
Hook-->>TimerPage: 상태 반영 (re-render)
TimerPage-->>User: 버튼 아이콘/레이블 갱신
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 0
🧹 Nitpick comments (8)
src/layout/components/header/StickyTriSectionHeader.tsx (2)
84-96: 헤더 버튼 패딩 추가는 OK. 접근성/의도 명시를 위해 title/aria/type을 함께 지정하세요.아이콘 버튼은 폼 내부에 들어갈 가능성도 있어 기본 type=submit을 피하는 것이 안전하며, 보조기기용 레이블을 제공하는 편이 좋습니다.
- <button - className="flex h-full items-center justify-center p-[4px]" + <button + type="button" + title="홈" + aria-label="홈으로 이동" + className="flex h-full items-center justify-center p-[4px]"
99-111: 로그인/로그아웃 버튼도 접근성 레이블과 type 지정이 필요합니다.상태에 따라 동적으로 레이블을 바꾸면 툴팁/스크린리더 모두 일관됩니다.
- return ( - <button - className="flex h-full items-center justify-center p-[4px]" + return ( + <button + type="button" + title={isLoggedIn() ? '로그아웃' : '로그인'} + aria-label={isLoggedIn() ? '로그아웃' : '로그인'} + className="flex h-full items-center justify-center p-[4px]"src/components/icons/Help.tsx (1)
26-30: 하드코딩된 흰색 스트로크는 테마 충돌 위험. color를 따르도록 변경 권장.밝은 배경/라이트 테마에서 흰색 스트로크가 사라질 수 있습니다.
- <path + <path d="M15.4339 20.4836H15.4358C16.3579 20.4621 17.176 21.2572 17.1975 22.2033C17.1749 23.1934 16.3565 23.9864 15.4358 23.965H15.4339C14.5061 23.9852 13.7318 23.2873 13.6741 22.3869L13.6721 22.2043V22.2004C13.632 21.3177 14.3449 20.5617 15.2503 20.4885L15.4339 20.4836ZM15.7444 8.0578H15.7454C17.2403 8.04689 18.5103 8.43236 19.4055 9.14471C20.2996 9.8562 20.8235 10.8973 20.8235 12.2092C20.8235 13.0954 20.5998 13.8153 20.1995 14.4172C19.7988 15.0196 19.2193 15.5065 18.5051 15.924L18.5042 15.925C17.9047 16.2913 17.4605 16.6552 17.1643 17.1232C16.8678 17.5921 16.7223 18.1611 16.7112 18.9299L16.6985 18.9963C16.6727 19.058 16.6114 19.1008 16.5403 19.1008H14.1526C14.0581 19.1007 13.9817 19.0243 13.9817 18.9299C13.9817 17.5851 14.1634 16.7157 14.5032 16.0822C14.8002 15.5286 15.2205 15.151 15.7561 14.7922L15.9924 14.6389C16.9222 14.0412 17.6515 13.3649 17.6516 12.342C17.6516 11.7757 17.4357 11.3145 17.0823 10.9924C16.7294 10.6708 16.2423 10.49 15.7014 10.4787H15.6995C14.7663 10.4994 13.8851 11.0742 13.6946 12.1682C13.6691 12.3133 13.5503 12.424 13.4104 12.424H10.9827C10.833 12.424 10.7139 12.3012 10.7249 12.1555C10.8312 10.7641 11.426 9.73588 12.3215 9.0578C13.2182 8.37884 14.4207 8.04721 15.7444 8.0578Z" - fill={color} - stroke="white" - strokeWidth="0.1" + fill={color} + stroke={color} + strokeWidth="0.1"src/page/TimerPage/TimerPage.tsx (2)
86-91: 도움말 버튼에 type/aria-label 추가 권장.툴팁 외에도 스크린리더 친화적인 레이블을 제공하고 submit 기본값을 방지하세요.
- <button - className="flex h-full items-center justify-center p-[4px]" - title="도움말" + <button + type="button" + className="flex h-full items-center justify-center p-[4px]" + title="도움말" + aria-label="도움말"
92-102: 전체 화면 토글 버튼에 접근성 속성(aria-pressed/label/type)을 추가하세요.토글 상태 노출과 폼 기본 동작 차단을 동시에 해결합니다.
- <button - className="flex h-full items-center justify-center p-[4px]" - title="전체 화면" - onClick={toggleFullscreen} - > + <button + type="button" + className="flex h-full items-center justify-center p-[4px]" + title={isFullscreen ? '전체 화면 종료' : '전체 화면'} + aria-label={isFullscreen ? '전체 화면 종료' : '전체 화면'} + aria-pressed={isFullscreen} + onClick={toggleFullscreen} + >src/page/TimerPage/components/FirstUseToolTip.tsx (1)
72-91: 툴팁 본문 내 아이콘은 장식용이므로 스크린리더에서 숨기세요.문장 중간의 SVG가 낭독되면 가독성이 떨어집니다.
- <RiFullscreenFill size={18} /> + <RiFullscreenFill size={18} aria-hidden focusable="false" /> ... - <RiFullscreenFill className="mx-[2px] self-center" /> + <RiFullscreenFill className="mx-[2px] self-center" aria-hidden focusable="false" /> ... - <RiFullscreenExitFill className="mx-[2px] self-center" /> + <RiFullscreenExitFill className="mx-[2px] self-center" aria-hidden focusable="false" />src/page/TimerPage/hooks/useTimerPageState.ts (2)
92-139: request 대상 엘리먼트를 페이지 전체(documentElement)로 고정한 선택은 합리적. 다만 컨텐츠 영역만 전체 화면으로 전환하는 옵션도 고려해 보세요.타이머 집중도를 높이려면 콘텐츠 컨테이너 ref에 대해 전체 화면을 요청하는 변형을 후속 과제로 제안합니다.
221-254: fullscreenchange 리스너 이펙트에 의존성 배열 누락(매 렌더 등록/해제). 초기 동기화도 필요.불필요한 add/remove가 반복됩니다. 최초 마운트 시 현재 상태를 즉시 반영하세요.
- useEffect(() => { + useEffect(() => { const handleFullscreenChange = () => { const doc = document as DocumentWithFullscreen; setIsFullscreen( !!( doc.fullscreenElement || doc.webkitFullscreenElement || doc.mozFullScreenElement || doc.msFullscreenElement ), ); }; document.addEventListener('fullscreenchange', handleFullscreenChange); document.addEventListener('webkitfullscreenchange', handleFullscreenChange); document.addEventListener('mozfullscreenchange', handleFullscreenChange); document.addEventListener('MSFullscreenChange', handleFullscreenChange); + // 초기 동기화 + handleFullscreenChange(); + return () => { document.removeEventListener('fullscreenchange', handleFullscreenChange); document.removeEventListener( 'webkitfullscreenchange', handleFullscreenChange, ); document.removeEventListener( 'mozfullscreenchange', handleFullscreenChange, ); document.removeEventListener( 'MSFullscreenChange', handleFullscreenChange, ); }; - }); + }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/components/icons/Help.tsx(1 hunks)src/components/icons/Home.tsx(1 hunks)src/components/icons/Login.tsx(1 hunks)src/layout/components/header/StickyTriSectionHeader.tsx(2 hunks)src/page/TimerPage/TimerPage.tsx(3 hunks)src/page/TimerPage/components/FirstUseToolTip.tsx(2 hunks)src/page/TimerPage/hooks/useTimerPageState.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/page/TimerPage/TimerPage.tsx (1)
src/components/icons/Help.tsx (1)
DTHelp(3-33)
🔇 Additional comments (2)
src/components/icons/Home.tsx (1)
10-22: 아이콘 뷰박스/스트로크 조정 LGTM.뷰박스/비율 및 strokeWidth 정리는 다른 아이콘들과 일관되고 문제 없습니다.
src/components/icons/Login.tsx (1)
10-43: 아이콘 업데이트 무리 없습니다.뷰박스/비율/경로 교체가 헤더 스케일링과 일관적입니다.
jaeml06
left a comment
jaeml06
left a comment
There was a problem hiding this comment.
넵 확인했습니다. 숀, 전체보기 전환 safari, chrome 두 환경 모두 잘 작동합니다. 다만 safari환경에서만 전체 화면 아이콘의 크기가 달라보이므로 이 부분만 수정해주시면 문제 없을 것 같습니다. 구현하느라 고생했습니다.
src/page/TimerPage/TimerPage.tsx
Outdated
| <RiFullscreenExitFill className="size-full" /> | ||
| ) : ( | ||
| <RiFullscreenFill className="size-full" /> | ||
| )} |
There was a problem hiding this comment.
기타 헤더의 아이콘은 svg를 이용한 벡터 이미지로 변경했으나 이 아이콘은 그렇지 못하여 safari환경에서는 혼자 아이콘이 작아보이는 문제가 있네요
There was a problem hiding this comment.
일단 아이콘은 react-icons의 것을 사용하기로 써니와 합의를 본 상태구요. 문제의 원인은 기존 헤더 아이콘들은 h-full만 적용하여 불필요하게 너비가 늘어나지 않게 제한하였는데, 이번 PR에서는 제가 size-full을 사용하여 너비에도 영향이 있었던 것 같네요. 이 부분 기본 헤더 아이콘과 동일하게 h-full로 통일하여 문제 수정하였습니다.
useon
left a comment
useon
left a comment
There was a problem hiding this comment.
숀 !! 전체 화면 기능 추가 정말 굿굿입니다 👍
다만 사파리에서 접속 시에는 전체 화면 아이콘만 이렇게 작아지는 문제와 비회원 모드 줄이 바뀌는 문제가 있네요 ..! 확인해 주시면 감사하겠습니다 !!!!
| handleFullscreenChange, | ||
| ); | ||
| }; | ||
| }); |
There was a problem hiding this comment.
헉 이건 누락한 게 맞습니다! 확인해주셔서 감사해요
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/page/TimerPage/hooks/useTimerPageState.ts (3)
95-138: toggleFullscreen을 useCallback으로 래핑하는 것을 고려하세요.
toggleFullscreen함수가 매 렌더링마다 재생성되고 있습니다. 이 함수가 다른 컴포넌트의 props나 다른 훅의 의존성으로 사용될 경우 불필요한 리렌더링을 발생시킬 수 있습니다.- const toggleFullscreen = async () => { + const toggleFullscreen = useCallback(async () => { const doc = document as DocumentWithFullscreen; const element = document.documentElement as HTMLElementWithFullscreen; try { const isCurrentlyFullscreen = doc.fullscreenElement || doc.webkitFullscreenElement || doc.mozFullScreenElement || doc.msFullscreenElement; if (!isCurrentlyFullscreen) { // 전체 화면 시작 (다양한 브라우저 지원) if (element.requestFullscreen) { await element.requestFullscreen(); } else if (element.webkitRequestFullscreen) { /* Safari */ await element.webkitRequestFullscreen(); } else if (element.mozRequestFullScreen) { /* Firefox */ await element.mozRequestFullScreen(); } else if (element.msRequestFullscreen) { /* IE11 */ await element.msRequestFullscreen(); } } else { // 전체 화면 종료 (다양한 브라우저 지원) if (doc.exitFullscreen) { await doc.exitFullscreen(); } else if (doc.webkitExitFullscreen) { /* Safari */ await doc.webkitExitFullscreen(); } else if (doc.mozCancelFullScreen) { /* Firefox */ await doc.mozCancelFullScreen(); } else if (doc.msExitFullscreen) { /* IE11 */ await doc.msExitFullscreen(); } } } catch (error) { console.error('# Failed to toggle fullscreen mode:', error); } - }; + }, []);
23-36: Fullscreen API 타입 정의를 별도 파일로 분리하는 것을 고려하세요.브라우저 호환성을 위한 타입 정의가 이 훅 파일 내에 직접 선언되어 있습니다. 재사용 가능성과 관심사 분리를 위해 별도의 타입 정의 파일로 분리하면 좋을 것 같습니다.
src/type/fullscreen.d.ts파일 생성:export interface DocumentWithFullscreen extends Document { mozCancelFullScreen?: () => Promise<void>; webkitExitFullscreen?: () => Promise<void>; msExitFullscreen?: () => Promise<void>; mozFullScreenElement?: Element; webkitFullscreenElement?: Element; msFullscreenElement?: Element; } export interface HTMLElementWithFullscreen extends HTMLElement { mozRequestFullScreen?: () => Promise<void>; webkitRequestFullscreen?: () => Promise<void>; msRequestFullscreen?: () => Promise<void>; }그리고 현재 파일에서:
-/** - * 브라우저별 접두사가 붙은 전체 화면 속성을 포함하는 사용자 정의 인터페이스 - */ -interface DocumentWithFullscreen extends Document { - mozCancelFullScreen?: () => Promise<void>; - webkitExitFullscreen?: () => Promise<void>; - msExitFullscreen?: () => Promise<void>; - mozFullScreenElement?: Element; - webkitFullscreenElement?: Element; - msFullscreenElement?: Element; -} - -interface HTMLElementWithFullscreen extends HTMLElement { - mozRequestFullScreen?: () => Promise<void>; - webkitRequestFullscreen?: () => Promise<void>; - msRequestFullscreen?: () => Promise<void>; -} +import type { DocumentWithFullscreen, HTMLElementWithFullscreen } from '../../../type/fullscreen';
136-136: 에러 로깅에서 더 구체적인 정보를 포함하세요.전체 화면 토글 실패 시 에러 객체만 로깅하고 있습니다. 디버깅을 위해 현재 상태와 시도한 작업을 함께 로깅하면 문제 해결에 도움이 될 것입니다.
- console.error('# Failed to toggle fullscreen mode:', error); + console.error('# Failed to toggle fullscreen mode:', { + error, + isCurrentlyFullscreen, + action: isCurrentlyFullscreen ? 'exit' : 'enter' + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/layout/components/header/StickyTriSectionHeader.tsx(3 hunks)src/page/TimerPage/TimerPage.tsx(3 hunks)src/page/TimerPage/hooks/useTimerPageState.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/layout/components/header/StickyTriSectionHeader.tsx
- src/page/TimerPage/TimerPage.tsx
🔇 Additional comments (1)
src/page/TimerPage/hooks/useTimerPageState.ts (1)
221-254: 전체 화면 이벤트 리스너의 의존성 배열이 비어있는 것이 적절합니다.기존 리뷰 코멘트("useon: 의존성 배열이 생략되어 있는 이유가 있나용 ?")와 관련하여, 이 경우에는 빈 의존성 배열
[]이 적절합니다.handleFullscreenChange함수 내에서 상태를 업데이트하는 로직이 외부 의존성 없이 DOM API만을 참조하므로, 컴포넌트 마운트 시 한 번만 설정되어야 합니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/icons/Help.tsx (4)
10-15: 뷰박스/종횡비(31×32) 변경 — 아이콘 세트와 정렬 일관성 확인 필요
- 다른 헤더 아이콘(Home/Login 등)도 31×32로 통일됐는지, 헤더에서 수직 정렬/여백이 흔들리지 않는지 20/24/28px 스케일에서 확인 부탁드립니다.
- 부모에서 width/height를 고정하면 Tailwind
aspect-[31/32]와 충돌할 수 있습니다. 둘 중 하나만 쓰는 사용 가이드를 제안합니다.- 선택: 가능하면 32×32(또는 공통 스퀘어 캔버스)로 정규화하면 픽셀 스냅이 더 안정적입니다.
20-24: 외곽 원: non-scaling-stroke 추가, miterlimit 제거 권장
- 원에는 miter 조인이 없어
strokeMiterlimit효용이 없습니다.- 다양한 크기로 스케일될 때 스트로크 두께가 과도하게 변하지 않도록
vectorEffect="non-scaling-stroke"추가를 권장합니다.- <path - d="M15.7693 29.8182C23.4009 29.8182 29.5875 23.6316 29.5875 16C29.5875 8.36843 23.4009 2.18182 15.7693 2.18182C8.13771 2.18182 1.9511 8.36843 1.9511 16C1.9511 23.6316 8.13771 29.8182 15.7693 29.8182Z" - stroke={color} - strokeWidth="2.7" - strokeMiterlimit="10" - /> + <path + d="M15.7693 29.8182C23.4009 29.8182 29.5875 23.6316 29.5875 16C29.5875 8.36843 23.4009 2.18182 15.7693 2.18182C8.13771 2.18182 1.9511 8.36843 1.9511 16C1.9511 23.6316 8.13771 29.8182 15.7693 29.8182Z" + stroke={color} + strokeWidth="2.7" + vectorEffect="non-scaling-stroke" + />
26-30: 내부 글리프 stroke=0.1은 저해상도에서 번짐/계단 현상 유발 가능
- fill+hairline stroke 조합은 작은 크기에서 안티앨리어싱 이슈가 잦습니다.
- 옵션 A(권장): stroke 제거.
- 옵션 B: 유지 시
vectorEffect="non-scaling-stroke"추가.옵션 A:
<path d="M15.4339 20.4836H15.4358C16.3579 20.4621 17.176 21.2572 17.1975 22.2033C17.1749 23.1934 16.3565 23.9864 15.4358 23.965H15.4339C14.5061 23.9852 13.7318 23.2873 13.6741 22.3869L13.6721 22.2043V22.2004C13.632 21.3177 14.3449 20.5617 15.2503 20.4885L15.4339 20.4836ZM15.7444 8.0578H15.7454C17.2403 8.04689 18.5103 8.43236 19.4055 9.14471C20.2996 9.8562 20.8235 10.8973 20.8235 12.2092C20.8235 13.0954 20.5998 13.8153 20.1995 14.4172C19.7988 15.0196 19.2193 15.5065 18.5051 15.924L18.5042 15.925C17.9047 16.2913 17.4605 16.6552 17.1643 17.1232C16.8678 17.5921 16.7223 18.1611 16.7112 18.9299L16.6985 18.9963C16.6727 19.058 16.6114 19.1008 16.5403 19.1008H14.1526C14.0581 19.1007 13.9817 19.0243 13.9817 18.9299C13.9817 17.5851 14.1634 16.7157 14.5032 16.0822C14.8002 15.5286 15.2205 15.151 15.7561 14.7922L15.9924 14.6389C16.9222 14.0412 17.6515 13.3649 17.6516 12.342C17.6516 11.7757 17.4357 11.3145 17.0823 10.9924C16.7294 10.6708 16.2423 10.49 15.7014 10.4787H15.6995C14.7663 10.4994 13.8851 11.0742 13.6946 12.1682C13.6691 12.3133 13.5503 12.424 13.4104 12.424H10.9827C10.833 12.424 10.7139 12.3012 10.7249 12.1555C10.8312 10.7641 11.426 9.73588 12.3215 9.0578C13.2182 8.37884 14.4207 8.04721 15.7444 8.0578Z" fill={color} - stroke={color} - strokeWidth="0.1" />옵션 B:
<path d="M15.4339 20.4836H15.4358C16.3579 20.4621 17.176 21.2572 17.1975 22.2033C17.1749 23.1934 16.3565 23.9864 15.4358 23.965H15.4339C14.5061 23.9852 13.7318 23.2873 13.6741 22.3869L13.6721 22.2043V22.2004C13.632 21.3177 14.3449 20.5617 15.2503 20.4885L15.4339 20.4836ZM15.7444 8.0578H15.7454C17.2403 8.04689 18.5103 8.43236 19.4055 9.14471C20.2996 9.8562 20.8235 10.8973 20.8235 12.2092C20.8235 13.0954 20.5998 13.8153 20.1995 14.4172C19.7988 15.0196 19.2193 15.5065 18.5051 15.924L18.5042 15.925C17.9047 16.2913 17.4605 16.6552 17.1643 17.1232C16.8678 17.5921 16.7223 18.1611 16.7112 18.9299L16.6985 18.9963C16.6727 19.058 16.6114 19.1008 16.5403 19.1008H14.1526C14.0581 19.1007 13.9817 19.0243 13.9817 18.9299C13.9817 17.5851 14.1634 16.7157 14.5032 16.0822C14.8002 15.5286 15.2205 15.151 15.7561 14.7922L15.9924 14.6389C16.9222 14.0412 17.6515 13.3649 17.6516 12.342C17.6516 11.7757 17.4357 11.3145 17.0823 10.9924C16.7294 10.6708 16.2423 10.49 15.7014 10.4787H15.6995C14.7663 10.4994 13.8851 11.0742 13.6946 12.1682C13.6691 12.3133 13.5503 12.424 13.4104 12.424H10.9827C10.833 12.424 10.7139 12.3012 10.7249 12.1555C10.8312 10.7641 11.426 9.73588 12.3215 9.0578C13.2182 8.37884 14.4207 8.04721 15.7444 8.0578Z" fill={color} stroke={color} strokeWidth="0.1" + vectorEffect="non-scaling-stroke" />
9-18: 접근성: 장식용 아이콘이면 스크린리더 숨김 처리
- 보통 Help 버튼 자체가 aria-label/tooltip을 제공하므로 아이콘은 중복 낭독을 피하는 게 안전합니다.
...props가 뒤에 있어 필요 시 오버라이드 가능합니다.<svg viewBox="0 0 31 32" fill="none" className={` aspect-[31/32] ${className} `} xmlns="http://www.w3.org/2000/svg" + aria-hidden="true" + focusable="false" {...props} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/icons/Help.tsx(1 hunks)
🔇 Additional comments (1)
src/components/icons/Help.tsx (1)
3-8: 디자인 갱신 반영 잘 됨
currentColor기본값 유지와IconProps전달 구조가 기존 사용처와 호환되어 보입니다. 변경 범위 내에서는 안전합니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/page/TimerPage/TimerPage.tsx (2)
17-17: react-icons 도입은 OK, 다만 아이콘 소스 일관성과 번들 영향 확인 권장기존 커스텀 SVG 아이콘들과 혼용 시 두께/스타일 불일치가 생길 수 있습니다. 추후 디자인 일관성을 위해 전체화면 아이콘도 내부 아이콘 컴포넌트로 편입하는 방안 고려해 주세요. 번들 관점에서도 react-icons 트리쉐이킹이 빌드 설정에 따라 비효율적일 수 있어 확인이 필요합니다.
92-102: 전체 화면 토글 버튼 접근성/UX 보완 제안토글 상태 노출과 단축키 힌트를 위해 접근성 속성 보강을 권장합니다. 상태에 따라 라벨/타이틀을 바꾸고, 토글 버튼 특성상
aria-pressed를 제공하세요. 폼 내부 사용 가능성에 대비해type="button"도 추가해 주세요.적용 예시:
- <button - className="flex aspect-square h-full items-center justify-center p-[4px]" - title="전체 화면" - onClick={toggleFullscreen} - > + <button + type="button" + className="flex aspect-square h-full items-center justify-center p-[4px]" + title={isFullscreen ? '전체 화면 종료' : '전체 화면'} + aria-label={isFullscreen ? '전체 화면 종료' : '전체 화면'} + aria-pressed={isFullscreen} + aria-keyshortcuts="f" + onClick={toggleFullscreen} + >간단 검증 체크리스트:
- 마우스 클릭/ESC/브라우저 UI로 전체 화면 전환 시 아이콘과 라벨이 즉시 동기화되는지
- Safari(macOS/iOS), Chrome, Firefox에서 버튼 크기/정렬이 균일한지
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/page/TimerPage/TimerPage.tsx(3 hunks)src/page/TimerPage/hooks/useTimerPageState.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/page/TimerPage/hooks/useTimerPageState.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/page/TimerPage/TimerPage.tsx (1)
src/components/icons/Help.tsx (1)
DTHelp(3-33)
🔇 Additional comments (2)
src/page/TimerPage/TimerPage.tsx (2)
34-44: isFullscreen/toggleFullscreen 공개 및 구조분해 사용 LGTM훅에서 상태/토글을 노출하는 방향 합리적입니다. 토글 핸들러는 useCallback으로 메모이제이션되어 불필요한 리렌더를 유발하지 않는지, ESC 등 네이티브 종료 시 fullscreenchange 이벤트로 isFullscreen이 정확히 동기화되는지만 한번 확인 부탁드립니다.
86-91: Safari 아이콘 크기 이슈 해결 확인 + 접근성 속성 추가 제안
h-full통일로 크기 불일치 이슈는 해소된 것으로 보입니다. 다만 title은 보조공학에서 신뢰도가 낮아aria-label과type="button"추가를 권장합니다.적용 예시:
- <button - className="flex h-full items-center justify-center p-[4px]" - title="도움말" - onClick={openUseTooltipModal} - > + <button + type="button" + className="flex h-full items-center justify-center p-[4px]" + title="도움말" + aria-label="도움말" + onClick={openUseTooltipModal} + >
jaeml06
left a comment
jaeml06
left a comment
There was a problem hiding this comment.
넵 기능 자체는 변경된 것 확인했습니다. 혹시 궁금한게 만약 전체 화면에서 다른 페이지로 이동할 경우, 전체화면이 유지 되는 것이 의도된 사항인지 궁금합니다. 전체화면인 상태에서 전환버튼이 없어진 경우, 당황할 수 있다고 생각이 드네요
useon
left a comment
useon
left a comment
There was a problem hiding this comment.
숀 !!! 꼼꼼하게 반영해 주셔서 감사합니다. 의존성 누락 부분과 사파리에서의 아이콘 수정도 확인했습니다 ! 치코 코멘트에서 저도 생각을 해 보았는데, 전체화면 해제 아이콘이 없는 화면에서 컴퓨터에서는 esc버튼을 누르면 해제할 수 있지만 아이패드나 이런 경우에는 전체화면 종료를 하기가 힘들 것 같아서 어떻게 하면 좋을지 고민되네요!!!
생각한 바로는 서비스의 모든 부분에 전체화면 아이콘을 삽입하기 보다는 토론 타이머 화면들에서만 되도록 하는 것이 좋다고 생각합니다,, 숀의 의견도 궁금합니다 !
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/hooks/useFullscreen.ts (1)
54-105: 훅 구현이 견고합니다.
setFullscreen함수가 DOM에서 직접 현재 상태를 읽어오는 것이 stale state 문제를 방지하는 좋은 접근입니다. 이벤트 리스너 정리도 올바르게 구현되었습니다.선택적 개선사항:
toggleFullscreen함수도setFullscreen처럼 DOM에서 직접 상태를 읽도록 리팩토링하면 stale closure 가능성을 완전히 제거할 수 있습니다:- const toggleFullscreen = useCallback(async () => { + const toggleFullscreen = useCallback(async () => { + const isCurrentlyFullscreen = !!getFullscreenElement(); const element = document.documentElement as HTMLElementWithFullscreen; - if (isFullscreen) { + if (isCurrentlyFullscreen) { await exitFullscreen(); } else { await enterFullscreen(element); } - }, [isFullscreen]); + }, []);다만 현재 구현도 실무에서 문제없이 작동할 것으로 판단됩니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/hooks/useFullscreen.ts(1 hunks)src/layout/components/header/StickyTriSectionHeader.tsx(5 hunks)src/page/TimerPage/TimerPage.tsx(4 hunks)src/page/TimerPage/hooks/useTimerPageState.ts(4 hunks)src/type/fullscreen.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/page/TimerPage/hooks/useTimerPageState.ts
- src/layout/components/header/StickyTriSectionHeader.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
src/page/TimerPage/TimerPage.tsx (1)
src/components/icons/Help.tsx (1)
DTHelp(3-33)
src/hooks/useFullscreen.ts (1)
src/type/fullscreen.d.ts (2)
DocumentWithFullscreen(4-11)HTMLElementWithFullscreen(13-17)
🔇 Additional comments (5)
src/type/fullscreen.d.ts (1)
1-17: 타입 정의가 정확하고 완전합니다.주요 브라우저(Firefox, Safari/Chrome, IE11)의 전체 화면 API를 모두 커버하는 타입 정의입니다. 메서드 시그니처와 optional 프로퍼티 처리가 적절합니다.
src/page/TimerPage/TimerPage.tsx (3)
86-92: 도움말 버튼 개선사항이 좋습니다.패딩 추가와 title 속성으로 접근성이 향상되었습니다.
93-103: 전체 화면 토글 버튼 구현이 적절합니다.조건부 아이콘 렌더링과 title 속성이 올바르게 구현되었습니다. 사용자가 현재 상태를 명확히 인지할 수 있습니다.
125-132: 전체 화면 해제 후 모달 열기는 훌륭한 UX 결정입니다.전체 화면 상태에서 모달을 열기 전에 전체 화면을 비활성화하여 모달이 가려지는 문제를 방지합니다. 사용자 경험 측면에서 올바른 접근입니다.
src/hooks/useFullscreen.ts (1)
8-52: 크로스 브라우저 지원과 에러 처리가 훌륭합니다.모든 주요 브라우저(Chrome, Safari, Firefox, IE11)의 vendor prefix를 지원하고, 각 비동기 작업에 try-catch 블록이 적절히 적용되었습니다.
|
/noti 추가 반영 사항치코의 의견
의도된 사항이 아닙니다! 치코 의견이 제가 놓친 부분을 지적해주신 것 같아서, 타이머 화면을 탈출하는 아래의 3가지 시나리오에 전체 화면 모드를 끄도록 하는 개선 사항을 적용했어요:
썬데이의 의견
저도 이 기능 구현할 당시 썬데이와 동일한 생각을 갖고 있었습니다. 따라서 현재 구현 기준으로는, 전체 화면 아이콘은 오직 타이머 페이지에서만 출력됩니다! 변경 사항남겨주신 댓글들을 참고하여 추가적인 변경 사항을 적용했습니다. 이에 따라 리뷰를 다시 요청드리오니, 바쁘시겠지만 한 번 재검토 진행해주시면 감사하겠습니다!
|
useon
left a comment
useon
left a comment
There was a problem hiding this comment.
숀 !! 전체 화면 토론 화면에서만 동작하도록 수정된 것 dev 환경에서 정상 작동 확인했습니다 감사합니다 ^.^ 로직 분리도 좋은 것 같아요 !! 함수명 관련해서 코멘트 남긴 것만 확인 부탁드려요 ^_^!!!
| }, [isFullscreen]); | ||
|
|
||
| // 값을 직접 입력하는 함수 | ||
| const setFullscreen = useCallback(async (value: boolean) => { |
There was a problem hiding this comment.
다른 곳에서 setFullscreen을 보고 state 값을 설정한다고 느껴졌어요!! 실제 역할은 단순 상태 변경이 아니고 set함수를 넘긴 것도 아니고 true/false에 따라 전체 화면을 켜고 끄는 함수 호출이니 의도를 담은 함수명이 좋을 것 같은데 어떻게 생각하시나요? 지금 당장 생각나는 것은 handleFullscreen과 같은 ,, ?!!?
i-meant-to-be
temporarily deployed
to
DEPLOY_DEV
GitHub Actions
Inactive
3 participants
🚩 연관 이슈
closed #378
📝 작업 내용
타이머에 전체 화면 기능 추가
그 외 변경 사항
🏞️ 스크린샷 (선택)
전체 화면 미적용
전체 화면 적용
최초 사용 시 도움말에 추가된 내용
🗣️ 리뷰 요구사항 (선택)
없음
Summary by CodeRabbit
신기능
문서화
스타일