Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature/BAR-139] Tooltip 컴포넌트 구현 #25

Merged
merged 7 commits into from
Jan 8, 2024
Merged

Conversation

dmswl98
Copy link
Member

@dmswl98 dmswl98 commented Jan 6, 2024

Summary

구현 내용 및 작업한 내역을 요약해서 적어주세요

image

To Reviewers

PR을 볼 때 주의깊게 봐야하거나 말하고 싶은 점을 적어주세요

  • z-index token도 상수화해두었는데 값이 적절한지 확인해주세요.
  • Minimal Tooltip의 경우, Hover시 Trigger 요소의 상위와 하위에 노출되어야 해요.
  • Highlight Tooltip의 경우, Hover시 Trigger 요소의 하위에만 노출되어야 해요.

How To Test

PR의 기능을 확인하는 방법을 상세하게 적어주세요

  • 스토리북 확인해주세요!

@dmswl98 dmswl98 self-assigned this Jan 6, 2024
Copy link

github-actions bot commented Jan 6, 2024

@miro-ring
Copy link
Contributor

@dmswl98
스크린샷 2024-01-06 오후 5 50 52
height 늘렸을 때 위치가 조금 다른 것 같은데 괜찮을까요?

Copy link
Member

@wonjin-dev wonjin-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 👍🏼

src/components/Tooltip/TooltipPortal.tsx Show resolved Hide resolved
src/styles/tokens.ts Outdated Show resolved Hide resolved
src/components/Tooltip/Tooltip.tsx Outdated Show resolved Hide resolved

useEffect(() => {
if (tooltipRoot) {
document.body.appendChild(tooltipRoot);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굉장히 기분탓이지만 리액트에서 동적으로 돔을 조작하는게 조금 거부감이 느껴지네요..

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wonjin-dev @dongkyun-dev

저도 그렇게 생각해요..🥲

우선 merge 후에 새로운 PR에서

  1. DOM을 직접 조작하지 않는 Portal 공통 컴포넌트를 구현하고,
  2. Modal과 Tooltip 컴포넌트에 적용하는 작업을 이어서 하겠습니다!

Copy link
Contributor

@miro-ring miro-ring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셧습니다!!!
이 PR 머지해주시면 말씀하신 DOM 처리 부분은 모달 PR에서 수정해서 반영해볼까요?

@dmswl98
Copy link
Member Author

dmswl98 commented Jan 8, 2024

@dongkyun-dev @wonjin-dev

추가로 작업한 부분은
height를 늘리거나 scroll시에도 Trigger 요소를 기준으로 올바른 위치에 Tooltip이 노출되도록 구현했어요.

@miro-ring
Copy link
Contributor

@dongkyun-dev @wonjin-dev

추가로 작업한 부분은 height를 늘리거나 scroll시에도 Trigger 요소를 기준으로 올바른 위치에 Tooltip이 노출되도록 구현했어요.

고생하셧습니다 ㅎㅎ

@dmswl98 dmswl98 added the feature label Jan 8, 2024
Copy link

github-actions bot commented Jan 8, 2024

@YAPP-Github YAPP-Github deleted a comment from github-actions bot Jan 8, 2024
@dmswl98 dmswl98 merged commit 2858061 into main Jan 8, 2024
2 of 3 checks passed
@dmswl98 dmswl98 deleted the feature/BAR-139 branch January 8, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants