refactor(tooltip): replace trigger prop and improve WCAG compliance by minji0214 · Pull Request #259 · sipe-team/side

minji0214 · 2026-04-27T14:06:41Z

Changes

파일 정리

useTooltip.tsx → useTooltip.ts 리네임

성능 개선

scroll/resize 이벤트 시 툴팁 위치 재계산을 requestAnimationFrame으로 변경

트리거 API 교체

기존: trigger: 'hover' | 'click' 단일 선택
변경: hover/focus 기본 활성
disableHoverListener, disableFocusListener로 각 인터랙션을 선택적으로 비활성화
click-only 동작은 controlled mode와 listener 비활성화 조합으로 대체

접근성 및 상호작용 개선

키보드 포커스만으로도 tooltip 내용을 확인할 수 있도록 개선
wrapper에 tabIndex={0}를 일괄 적용하지 않도록 조정, 키보드 탐색 시 불필요한 추가 포커스를 방지
asChild=true 사용 시 자식 요소의 기존 스타일을 그대로 유지
hover된 trigger에서 tooltip content로 마우스가 이동해도 즉시 닫히지 않도록 동작 개선

불필요한 코드 제거

isVisible && createPortal(...) 안에서 'visible' 클래스는 항상 true → 클래스 및 관련 CSS(opacity, transform, transition) 제거
position: 'fixed' 인라인 스타일 중복 제거 (CSS base에 이미 존재)
--tooltip-bg-color JS 폴백(|| '#000000') 중복 제거 (CSS var fallback으로 충분)

Visuals

2026-04-27.10.04.03.mov

Checklist

Have you written the functional specifications?
Have you written the test code?

Additional Discussion Points

찾아보다 보니, tooltip에서는 hover 만 지원하고, click action이 필요한 경우 popover 엄격하게 분리하는 경우도 있는듯 합니다.
아직 tooltip의 사용처가 명확하지 않아서 tooltip의 활용도에 따라서 popover의 도입을 고민해도 좋을거 같습니다.
참고 자료 : Tooltip vs Popover

- Remove always-true visible class from portal (portal only renders when isVisible=true) - Remove associated CSS opacity/transform/transition/pointerEvents overrides - Remove redundant position:fixed from inline styles (already in CSS base) - Remove redundant --tooltip-bg-color JS fallback (CSS already has var fallback) - Conditionally apply tabIndex=0 only when focus listener is active (WCAG 2.4.7) - Apply styles.button class only when asChild=false (WCAG 1.4.3)

esbuild 0.27 changed its behavior to refuse transforming syntax for old browser targets (es2020, chrome87). Storybook 8.5 manager build uses these hardcoded targets and breaks with 0.27. Downgrade to 0.24 as a temporary fix until Storybook is upgraded to support esbuild 0.27.

…build" This reverts commit e3e8a95.

Focus listener is always required for keyboard accessibility (WCAG 2.1.1). Removing this prop enforces accessible defaults and simplifies the API.

Uses @Keyframes instead of CSS transition to correctly animate on DOM insertion. Respects prefers-reduced-motion for accessibility (WCAG 2.3.3).

…led open state On first click, the browser fires focus before click, causing onFocus→requestOpen to set open=true before the onClick toggle sets it back to false. Fixed by tracking mousedown state so focus triggered by mouse click is ignored (keyboard focus still works).

…Index, spread rest props - Restore disableFocusListener prop for click-only controlled mode support - Remove hardcoded tabIndex={0} to respect consumer element focusability - Spread ...rest onto wrapper so consumers can pass tabIndex, role, aria-* etc. - Merge className from rest with internal styles via clsx

coderabbitai · 2026-04-27T14:06:51Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)

main
release/v1

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b2915578-07d4-4815-bfa0-bc9d93ae1110

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/apply-pr245-review-MinjiJeon

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

changeset-bot · 2026-04-27T14:06:52Z

🦋 Changeset detected

Latest commit: 2aa0bb9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@sipe-team/tooltip	Major
@sipe-team/side	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codecov · 2026-04-27T14:07:37Z

Codecov Report

❌ Patch coverage is 98.64499% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/tooltip/src/Tooltip.tsx	87.80%	5 Missing ⚠️

Files with missing lines	Coverage Δ
packages/tooltip/src/Tooltip.css.ts	`100.00% <100.00%> (ø)`
packages/tooltip/src/Tooltip.test.tsx	`100.00% <100.00%> (ø)`
...ackages/tooltip/src/hooks/useTooltip/useTooltip.ts	`100.00% <100.00%> (ø)`
packages/tooltip/src/Tooltip.tsx	`93.81% <87.80%> (-6.19%)`	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…id placement

G-hoon · 2026-04-28T08:13:59Z

@@ -0,0 +1,5 @@
+---
+"@sipe-team/tooltip": minor


이거 minor 가 아니라 major 로 바꿔주세요 !
trigger prop 이 제거되어,
기존 사용자는 코드가 깨집니다.
아니면, trigger prop 을 deprecated 로 유지하면서 변경 사항과 병행되는 것도 고려되면 좋을 것 같습니다.

G-hoon · 2026-04-28T08:19:41Z

-        tabIndex={trigger === TooltipTrigger.click ? 0 : undefined}
-        className={styles.button}
+        className={clsx(asChild ? undefined : styles.button, className)}
+        {...triggerHandlers}


이거 ...rest 랑 ...triggerHandlers 둘다 있는게 좋지 않은 것 같은데...
만약 사용자가, onMouseEnter 를 rest 에 넘기면, triggerHandlers 의 onMouseEnter 가 덮어쓰여질 것 같습니다.

이게 의도된 동작이라면 상관없는데.
제 생각엔, 둘이 우선순위를 조정하여 합성하는 내용이 있으면 유지보수 측면에서도 더 좋을 것 같고, 사용자도 더 안전하게 사용할 수 있을 것 같습니다.

이부분은 버그인듯 합니다. composeHandlers 헬퍼를 추가해서 사용 방식을 변경하였습니다.

G-hoon · 2026-04-28T08:24:09Z

+const fadeIn = keyframes({
+  from: { opacity: 0, transform: 'scale(0.95)' },
+  to: { opacity: 1, transform: 'scale(1)' },
+});


이거 fadeIn 만 있고, fadeOut 은 없는 이유가 있을까요....?
구조상 isVisible && createPortal이라 DOM이 바로 제거되니 fadeOut이 어려울 것 같긴 한데, 향후 고려 사항이 있는지 궁금합니다.

복잡성이 더해질거 같아 별도의 계획이 없었는데 필요하다면 이슈로 관리해 가도 좋을거 같습니다!

G-hoon · 2026-04-28T08:29:00Z

+
+  const requestOpen = useCallback(() => {
+    clearCloseTimer();
+    setInternalOpen(true);


controll mode 일 때는, 의미 없는 것 같습니다.
if (!isControlled) setInternalOpen(true);
로 수정하면 좋을 것 같습니다.

G-hoon · 2026-04-28T08:35:37Z

+
+interface UseTooltipProps {
+  placement: TooltipPosition;
+  gap: number;


이거, placement 랑 gap 이 꼭 required 여야 할 이유가 있을까요...?
hook 자체에서 기본값
gap = 8, placement = 'top' 가지고 있으면
사용하는 쪽에서 더 편하게 사용할 수 있을 것 같습니다.
사용하는 쪽에서, gap 이랑 placement 를 꼭 정의해야 할 관심사라고 느껴지진 않는 것 같아요.

의견 감사합니다. 현재는 hook이 내부 구현용이라 required로 두었지만, 추후 hook 재사용/확장 가능성까지 고려하면 placement, gap을 optional로 두고 hook 내부 기본값으로 처리하는 방향도 맞다고 생각해서 옵셔널로 변경하였습니다

Yeom-JinHo · 2026-04-28T15:03:51Z

리뷰하고 싶은 포인트들을 지훈님이 잘 코멘트 달아주셨네용 😆

- Change changeset from minor to major (trigger prop removal is breaking change) - Compose user event handlers with internal handlers via composeHandlers helper - Make placement and gap optional in useTooltip with default values

…lled mode Co-Authored-By: MinjiJeon <jinnyjeon@sweetspot.co.kr>

osohyun0224

지훈님 리뷰 잘 반영된 것 같아서 승인합니다~!

KYBee

작업 고생하셨습니다~ 코멘트도 잘 반영해주셔서 좋습니다 👍🏻

minji0214 added 13 commits April 26, 2026 23:53

refactor(tooltip): rename useTooltip.tsx to useTooltip.ts

281c739

perf(tooltip): throttle scroll/resize position update with rAF

de9e462

feat(tooltip): support keyboard focus for hover trigger

099e90c

refactor(tooltip): replace trigger prop with MUI-style listener controls

5db949b

fix(tooltip): remove redundant isVisible condition in portal className

c75f849

Revert "chore: downgrade esbuild to ~0.24.0 to fix Storybook manager …

a7b7491

…build" This reverts commit e3e8a95.

refactor(tooltip): remove disableFocusListener prop

3634a72

Focus listener is always required for keyboard accessibility (WCAG 2.1.1). Removing this prop enforces accessible defaults and simplifies the API.

feat(tooltip): add fade-in animation on mount

272395b

Uses @Keyframes instead of CSS transition to correctly animate on DOM insertion. Respects prefers-reduced-motion for accessibility (WCAG 2.3.3).

chore: add changeset for tooltip API refactor

d4b8b04

minji0214 self-assigned this Apr 27, 2026

minji0214 requested review from 3o14, G-hoon, KYBee, Yeom-JinHo, froggy1014, haedawn, minjeeki and osohyun0224 as code owners April 27, 2026 14:06

test(tooltip): add missing coverage for mouseUp, scroll/resize, inval…

d1284ef

…id placement

Base automatically changed from dev/side-v2 to main April 27, 2026 14:36

Yeom-JinHo changed the base branch from main to dev April 27, 2026 14:56

G-hoon reviewed Apr 28, 2026

View reviewed changes

minji0214 and others added 2 commits April 29, 2026 00:15

refactor(tooltip): apply PR review feedback

68c5402

- Change changeset from minor to major (trigger prop removal is breaking change) - Compose user event handlers with internal handlers via composeHandlers helper - Make placement and gap optional in useTooltip with default values

fix(tooltip): guard setInternalOpen with isControlled check in contro…

2aa0bb9

…lled mode Co-Authored-By: MinjiJeon <jinnyjeon@sweetspot.co.kr>

osohyun0224 approved these changes Apr 29, 2026

View reviewed changes

minji0214 mentioned this pull request Apr 29, 2026

docs : update minjijeon week log sipe-team/5-1_side_version_2#22

Open

KYBee approved these changes Apr 29, 2026

View reviewed changes

minji0214 merged commit d094f2c into dev Apr 29, 2026
5 checks passed

minji0214 deleted the fix/apply-pr245-review-MinjiJeon branch April 29, 2026 14:20

Conversation

minji0214 commented Apr 27, 2026

Changes

Visuals

Checklist

Additional Discussion Points

Uh oh!

coderabbitai Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

changeset-bot Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

codecov Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yeom-JinHo commented Apr 28, 2026

Uh oh!

osohyun0224 left a comment

Choose a reason for hiding this comment

Uh oh!

KYBee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coderabbitai Bot commented Apr 27, 2026 •

edited

Loading

changeset-bot Bot commented Apr 27, 2026 •

edited

Loading

codecov Bot commented Apr 27, 2026 •

edited

Loading