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] 버튼 컴포넌트 에러 / 로직 수정 & 새 버튼 구현 #58

Merged
merged 16 commits into from
Jul 8, 2024

Conversation

jeeminyi
Copy link
Member

@jeeminyi jeeminyi commented Jul 8, 2024

작업 내용 🧑‍💻

  • 승연이와 버튼 파일명이 같아 변경하였습니다. arrange btn -> sort btn으로 수정하였습니다 (아래 이미지 참고)
  • 버튼 컴포넌트 띰 에러, 린팅 에러 수정했습니다
  • theme 에 text btn 파일 분리해두었습니다.
  • text btn disabled / abled 구현해두었습니다.
  • 변경된 디자인 컴포넌트 반영하여 버튼 구현했습니다.

알게된 점 🚀

기록하며 개발하기!

  • css 와 styled 적절히 쓰기.., 이건 더 공부해보겠습니다. css가 아닌 styled를 썼더니 미친 에러의 굴레에 빠져버렸습니다.
  • isActive 속성을 boolean으로 두어 light속성에 해당하는 버튼이더라도 페이지에 따라 호버 / 클릭 스타일이 자유롭게 적용가능하도록 했습니다. false로 두면 disabled과 동일하다고 보시면 됩니다!

리뷰 요구사항 💬

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요

  • hover bar btn을 새롭게 구현하였는데요, 아무리 동일한 icon을 svg로 export 해도, 피그마에 보이는 아이콘과 추출 아이콘이 다르다는 문제가 있었습니다. 해당 이슈 관련하여 아시는 분들은 피드백 부탁드리겠습니다.. (디자인 파트 오면 질문할 예정) 아래 이미지 참고해주시면 됩니다!!

관련 이슈

close #54

스크린샷 (선택)

arrange btn -> sort btn으로 수정한 컴포넌트 입니다!
스크린샷 2024-07-08 오후 7 02 59

피그마 이미지
스크린샷 2024-07-08 오후 7 00 36
제가 추출한 것...
스크린샷 2024-07-08 오후 7 01 30

Copy link
Contributor

@Kjiw0n Kjiw0n left a comment

Choose a reason for hiding this comment

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

진~~~짜 양 많은 공통버튼들 구현하느라 고생 많으셨습니다!!
hover랑 pressed 등이 적용이 되는 버튼, 안되는 버튼 등등 여러가지 상황들 모두 고려해서 최대한 쓰기 쉽게 만들어주신 것 같아요 굳굳

Copy link
Member

@wrryu09 wrryu09 left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다!!! 버튼의 굴레에서 해방~!!

Comment on lines 16 to 28
const DeleteBtnCss = css`
display: flex;
align-items: center;
justify-content: center;
width: 3.2rem;
height: 3.2rem;

border-radius: 10px;
`;

const DeleteBtnLayout = styled.button`
${DeleteBtnCss}
`;
Copy link
Member

Choose a reason for hiding this comment

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

DeleteBtnLayout${DeleteBtnCss} 를 받는 역할만 하고 있어서 css 분리할 필요 없이 DeleteBtnLayout 안에 스타일 코드 넣어줘도 괜찮을 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

코드리뷰 반영해두었습니다!! 매번 꼼꼼한 리뷰 달아주셔서 감사합니다 ㅎㅎ

Comment on lines 44 to 45
width: 1.391rem;
height: 0.9563rem;
Copy link
Member

Choose a reason for hiding this comment

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

각각 1.4rem, 1rem 정도로 작성해도 될 것 같습니다

Comment on lines 33 to 57
background-color: ${({ theme }) => theme.palette.Blue.Blue1};

path {
fill: ${({ theme }) => theme.palette.Primary};
}

${({ isHover, theme }) =>
isHover &&
css`
&:hover {
background-color: ${theme.palette.Blue.Blue3};
}
`}
${({ isPressed, theme }) =>
isPressed &&
css`
&:active {
background-color: ${theme.palette.Primary};

path {
fill: ${theme.palette.Grey.White};
}
}
`}
`;
Copy link
Member

Choose a reason for hiding this comment

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

이쪽 스타일이 유사하게 세팅버튼 모두에 적용되는 것 같아서 묶어서 모든 세팅버튼에 재사용해도 좋을 것 같아요!

Comment on lines 24 to 28
height: 2.4rem;
padding: 0.4rem;

border: 1px solid var(${({ theme }) => theme.palette.WITHE});
border-radius: 10px;
border: 1px solid ${({ theme }) => theme.palette.Grey.White};
border-radius: 12px;
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

Choose a reason for hiding this comment

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

테마파일도 따로 빼셨군요~~ 굿!!

Copy link
Member

@seong-hui seong-hui left a comment

Choose a reason for hiding this comment

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

굉장히 많은 버튼이 존재해서 작업하기 힘드셨을텐데 하나하나 꼼꼼하게 너무 수고하셨습니당 덕분에 버튼 편하게 잘 사용할 수 있겟네요 👍

}

const Check1Btn = ({ type }: Check1) => {
function Check1Btn({ type, isHover, isPressed }: Check1Props) {
Copy link
Member

Choose a reason for hiding this comment

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

hover나 active의 상황도 props로 받는 과정으로 수정하셨군요! 혹시 해당 내용을 props로 받아서 관리해야 하는지 궁금합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

네! 호버가 되지 않는 경우도 있어서 다 따로 분리하였습니다!
스타일 적용이 되어야 할 때는 ishover/isPressed로 두시고, 적용이 되면 안될 때는 isHover={false} isPressed={false}로 두시면 됩니다!!

Copy link
Member

Choose a reason for hiding this comment

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

아하 이게 그 같은 버튼이라도 호버가 될 때가 있고 아닐때도 있다는 부분이었군요..! 까다로운 처리였을텐데 너무나 대단하십니당

@@ -7,14 +7,14 @@ interface RefreshProps {
isDisabled: boolean;
}

const RefreshBtn = ({ isDisabled }: RefreshProps) => {
function RefreshBtn({ isDisabled }: RefreshProps) {
Copy link
Member

Choose a reason for hiding this comment

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

disabled props로 받아서 처리해야함을 고민하시더니 결국 해내셨군요 짱짱

Copy link
Member Author

Choose a reason for hiding this comment

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

ㅋㅋㅋ 옆에서 힌트 많이 주셔서,,^^ 감쟈합니다!!

@wrryu09
Copy link
Member

wrryu09 commented Jul 8, 2024

수정사항 확인 완료했습니다~~!! 너무너무너무 고생 많으셧어요!!!

@jeeminyi jeeminyi merged commit ba31164 into develop Jul 8, 2024
2 checks passed
@Kjiw0n Kjiw0n deleted the fix/#54/folder-rename branch July 8, 2024 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FIX] Arrange Btn 폴더명 & 조건문 구조 수정
4 participants