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

[Fix/BAR-250] 폴더 삭제 에러 해결 #79

Merged
merged 13 commits into from
Feb 23, 2024
Merged

[Fix/BAR-250] 폴더 삭제 에러 해결 #79

merged 13 commits into from
Feb 23, 2024

Conversation

dmswl98
Copy link
Member

@dmswl98 dmswl98 commented Feb 22, 2024

Summary

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

To Reviewers

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

  • (TooltipButton): 중복 button 태그 제거
  • TooltipButton 컴포넌트를 생성해 리팩토링하는 과정에서 내부 button tag를 작성하지 않아 모든 카드의 아이콘 버튼이 클릭되지 않는 문제가 있었어요.
  • 드롭다운 아이콘 버튼인 경우, Dropdown.Trigger(button tag) > Tooltip.Trigger(div tag)로 합성해 사용하고,
  • 아이콘 버튼인 경우 Tooltip.Trigger(div tag)를 사용하다보니
  • TooltipButton 컴포넌트를 설계하는 과정에서 button tag를 생략해 작성했던 것이 문제였어요.
  • Tooltip.Trigger를 다형성 컴포넌트로 구현하면 TooltipButton 내부 로직이 쉽게 정리될 것 같은데...! 시간이 부족한 관계로 분기 처리로 대신 해결해두었어요.

How To Test

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

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.

@dmswl98
고생하셨습니다 ㅎㅎ

Comment on lines 10 to 12
':disabled': {
cursor: 'not-allowed',
},
Copy link
Contributor

Choose a reason for hiding this comment

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

보통 not-allowed 처리 안하지 않나요?? 디자인쪽 니즈인가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

7294d1f 하는게 더 명확해보일거라 생각했는데 실무에서는 잘 모르겠네요ㅎㅎ

Comment on lines 65 to 73
return (
<>
{!defaultIsVisibleMenu ? (
isVisibleMenu && <div className={styles.menu}>{children}</div>
) : (
<div className={styles.menu}>{children}</div>
)}
</>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

if (!defaultIsVisibleMenu && !isVisibleMenu) return null;
return <div></div>

해당 형태가 더 깔끔할 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

1273950

해당 부분 이렇게 수정해두었습니다~

return (
    <>
      {(defaultIsVisibleMenu || isVisibleMenu) && (
        <div className={styles.menu}>{children}</div>
      )}
    </>
  );  

@@ -16,10 +16,11 @@ interface CardProps {
| 'grey'
| 'white';
className?: string;
defaultIsVisibleMenu?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

boolean 타입의 변수명이 아래쪽에서 is로 시작해서 이 변수명도 is로 시작해도 좋을 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

고것도 고민해봤는데 default가 중간에 들어가면 네이밍이 어색할 거 같았고 IsVisibleMenu가 뭉쳐있어야 이해하기 쉬울 것 같아 요렇게 해두었습니다..!

Comment on lines +49 to +54
onMouseEnter={() => {
!defaultIsVisibleMenu && onShow();
}}
onMouseLeave={() => {
!defaultIsVisibleMenu && onHide();
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 쓰이는 조건문 필요한가요?

@dmswl98 dmswl98 merged commit 4b647ce into main Feb 23, 2024
@dmswl98 dmswl98 deleted the fix/BAR-250 branch February 23, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants