[AI] feat: 이슈 #36, #37, #20 구현#38
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request enhances the ScrollManager with focus management, sticky element offsets, and custom easing functions. Key feedback includes fixing a string comparison bug in sticky height calculation and correcting the scroll target calculation to subtract sticky height instead of adding it. Suggestions were also made to improve accessibility by refining ARIA attributes and to enhance type safety for the easing function to avoid potential runtime errors.
| if (element) { | ||
| const rect = element.getBoundingClientRect(); | ||
| const style = window.getComputedStyle(element); | ||
| if (style.position === ' sticky' || style.position === 'fixed') { |
There was a problem hiding this comment.
문자열 비교 시 ' sticky'와 같이 앞에 공백이 포함되어 있습니다. window.getComputedStyle(element).position은 공백 없이 'sticky'를 반환하므로, 이 조건문은 항상 거짓이 되어 sticky 요소의 높이가 계산되지 않는 버그가 발생합니다.
| if (style.position === ' sticky' || style.position === 'fixed') { | |
| if (style.position === 'sticky' || style.position === 'fixed') { |
|
|
||
| const targetScrollTop = | ||
| elementRect.top + this.currentScrollTop - rootRect.top + this.options.offset; | ||
| elementRect.top + this.currentScrollTop - rootRect.top + this.options.offset + stickyHeight; |
There was a problem hiding this comment.
stickyHeight를 타겟 스크롤 위치에 더하고 있는데, 이는 빼야 합니다. sticky 헤더가 있는 경우 해당 높이만큼 덜 스크롤해야 타겟 요소가 헤더에 가려지지 않습니다.
| elementRect.top + this.currentScrollTop - rootRect.top + this.options.offset + stickyHeight; | |
| elementRect.top + this.currentScrollTop - rootRect.top + this.options.offset - stickyHeight; |
| } | ||
|
|
||
| element.setAttribute('role', 'region'); | ||
| element.setAttribute('aria-labelledby', id); |
| /** sticky 요소들의 ID 또는 element 배열입니다. 스크롤 위치 계산 시これらの 높이가 오프셋에 추가됩니다 */ | ||
| stickyElements?: string[] | HTMLElement[]; | ||
| /** 커스텀 easing 함수입니다. t: 0~1 사이의 진행률, 반환값: 변환된 진행률 */ | ||
| easing?: (t: number) => number | undefined; |
| rootMargin: '-20% 0px -60% 0px', | ||
| focusActiveSection: false, | ||
| stickyElements: [], | ||
| easing: undefined as unknown as (t: number) => number | undefined, |
| const animate = (currentTime: number) => { | ||
| const elapsed = currentTime - startTime; | ||
| const progress = Math.min(elapsed / duration, 1); | ||
| const easedProgress = easing(progress)!; |
- focusActiveSection, stickyElements, easing 옵션 설명 추가 - 새로운 메서드들 (scrollToNext, scrollToPrev 등) 추가 - Advanced Usage 예제 추가 - EN/KO 문서 모두 업데이트
리뷰변경 사항들을 확인했습니다. 구현된 기능
테스트
제안
결론: 승인 가능합니다. |
- scrollToNext/Prev 테스트 추가 - onProgressChange 테스트 추가 -各种 엣지 케이스 테스트 추가 - 51 tests로 증가
- scrollToFirst/Last 테스트 - stickyElements HTMLElement 배열 테스트 - 52 tests로 증가 - 포맷 수정
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly expands the ScrollManager functionality, adding support for custom easing, sticky element offsets, focus management, and accessibility attributes like role="region". It also introduces new methods for navigation and state tracking, along with updated documentation and tests. Review feedback identified critical bugs, including an incorrect string comparison for sticky positions and a logic error where sticky heights were added instead of subtracted from the scroll target. Additionally, improvements were suggested for ARIA attribute implementation, type safety in easing functions, and more precise sticky element height calculations.
| if (element) { | ||
| const rect = element.getBoundingClientRect(); | ||
| const style = window.getComputedStyle(element); | ||
| if (style.position === ' sticky' || style.position === 'fixed') { |
There was a problem hiding this comment.
|
|
||
| const targetScrollTop = | ||
| elementRect.top + this.currentScrollTop - rootRect.top + this.options.offset; | ||
| elementRect.top + this.currentScrollTop - rootRect.top + this.options.offset + stickyHeight; |
There was a problem hiding this comment.
stickyHeight를 더하는 대신 빼야 합니다. 스티키 헤더가 있는 경우, 섹션이 헤더에 가려지지 않도록 스크롤 위치를 헤더 높이만큼 위로(작은 값으로) 조정해야 하기 때문입니다. 현재처럼 더하게 되면 섹션이 헤더 아래로 더 가려지게 됩니다.
| elementRect.top + this.currentScrollTop - rootRect.top + this.options.offset + stickyHeight; | |
| elementRect.top + this.currentScrollTop - rootRect.top + this.options.offset - stickyHeight; |
| /** sticky 요소들의 ID 또는 element 배열입니다. 스크롤 위치 계산 시これらの 높이가 오프셋에 추가됩니다 */ | ||
| stickyElements?: string[] | HTMLElement[]; | ||
| /** 커스텀 easing 함수입니다. t: 0~1 사이의 진행률, 반환값: 변환된 진행률 */ | ||
| easing?: (t: number) => number | undefined; |
| rootMargin: '-20% 0px -60% 0px', | ||
| focusActiveSection: false, | ||
| stickyElements: [], | ||
| easing: undefined as unknown as (t: number) => number | undefined, |
| for (const el of this.options.stickyElements) { | ||
| const element = typeof el === 'string' ? document.getElementById(el) : el; | ||
| if (element) { | ||
| const rect = element.getBoundingClientRect(); | ||
| const style = window.getComputedStyle(element); | ||
| if (style.position === ' sticky' || style.position === 'fixed') { | ||
| totalHeight += rect.height; | ||
| } | ||
| } | ||
| } |
| } | ||
|
|
||
| element.setAttribute('role', 'region'); | ||
| element.setAttribute('aria-labelledby', id); |
| const animate = (currentTime: number) => { | ||
| const elapsed = currentTime - startTime; | ||
| const progress = Math.min(elapsed / duration, 1); | ||
| const easedProgress = easing(progress)!; |
- ' sticky' → 'sticky': 앞 공백으로 인해 항상 false였던 문자열 비교 버그 수정 - stickyHeight 덧셈 → 뺄셈: sticky 헤더 높이는 타겟 스크롤 위치에서 빼야 함 - 상단 고정 요소만 계산: rect.top <= 0 조건으로 하단 sticky 요소 제외 - aria-labelledby → aria-label: 자기 참조는 접근성 표준에 부적합 - easing 반환 타입: number | undefined → number (NaN 방지) - easing non-null assertion 제거: 타입 수정으로 불필요해진 ! 제거 - JSDoc 한국어로 수정 (일본어 혼입 제거) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Changes
Test Results
🤖 Generated by jump-section AI Pipeline