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

[ Refactor ] 툴팁 공통 컴포넌트로 분리 #348

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

kimmoonju-102
Copy link
Collaborator

🔥 Related Issues

✅ 작업 내용

  • 툴팁 공통 컴포넌트로 분리 시킴

📸 스크린샷 / GIF / Link

📌 이슈 사항

1️⃣ 툴팁 공통 컴포넌트로 분리

필요한 props를 전달해서 사용할 수 있게 분리해뒀습니다!

const TooltipInfo = ({ isVisible, onClick, text }: TooltipProps) => {
  return (
    <TooltipContainer $isVisible={isVisible}>
      <Text>{text}</Text>
      <TooltipClose type="button" onClick={onClick}>
        <IcCancelSmallWhite />
      </TooltipClose>
    </TooltipContainer>
  );
};
<TooltipInfo
  isVisible={isTooltipVisible}
  onClick={handleTooltipClose}
   text="주간 성과율은 월요일마다 초기화 돼요"
 />

✍ 다음 이슈에서 작업할 예정

  1. InformationTooltip 공통 컴포넌트가 있더라구요. 툴팁 코드들이 반복되고 있기 때문에 InformationTooltip 이용해서 수정할 예정입니다.
    스크린샷 2025-01-13 211644
    스크린샷 2025-01-13 211636
    스크린샷 2025-01-13 212344

  2. font-size 수정
    font-size: ${({ theme }) => theme.fonts.body_ligth_12}; => 이렇게 정의 되어있는 부분들이 있어서 수정할 예정입니다.

Copy link
Member

@Arooming Arooming left a comment

Choose a reason for hiding this comment

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

일단 공통 컴포넌트 분리하는건 내부적인 코드 개선이고, 폰트가 제대로 적용되지 않는 이슈는 사용자에게 직접적으로 보여지는 부분이기 때문에 개인적으로는 폰트 관련 이슈부터 처리하는게 좋을 것 같습니다 !!


white-space: nowrap;
transform: translate(8px, -50%);
opacity: ${({ $isVisible }) => ($isVisible ? 1 : 0)};
Copy link
Member

Choose a reason for hiding this comment

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

ASK: visibility가 visible이면 화면에 모두 보이게 되고, hidden이면 아예 안 보이게 되는데 opacity를 따로 설정해주신 이유가 있을까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • visible 일 떄 opacity 를 줄려고 한 것 같네용

</TitleContainer>

<Notic>
<BtnInformation />
<Tooltip>
<Tooltips>
Copy link
Member

Choose a reason for hiding this comment

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

P5 + ASK: 이건 개인적으로 궁금한건데..! 툴팁은 하나인데 기존 Tooltip이라는 이름에서 Tooltips로 변경한 이유가 뭔지 알 수 있을까요!?

onClick: () => void;
}

const TooltipInfo = ({ isVisible, onClick, text }: TooltipProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

P2: TooltipInfo라는 이름은 뭔가.. 툴팁에 대한 정보를 나타내는 느낌이 강해서 그것보다는 해당 컴포넌트의 역할을 직관적으로 나타낼 수 있는 네이밍을 사용해보는건 어떨까요!!?
ex. GuideTooltip, DetailTooltip ... 등등

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • 전 그냥 Tooltip 으로 바꾸는게 좋을듯합니당

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants