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

[FEAT] 공통 컴포넌트 버튼 퍼블리싱 #40

Merged
merged 21 commits into from
Jul 7, 2024

Conversation

jeeminyi
Copy link
Member

@jeeminyi jeeminyi commented Jul 7, 2024

작업 내용 🧑‍💻

  • globalstyle에 button css 값 초기화
  • 버튼 컴포넌트 추가

알게된 점 🚀

기록하며 개발하기!

  • svg를 매번 하나하나 import 했었는데 Icons 함수 안에 객체로 아이콘들을 전부 선언해주어서 필요할 때마다 꺼내쓰기 좋았다. import 문도 줄어들어서 보기 편했다. ㅎㅎ
  • 컴포넌트 분리 / 재사요에 대한 니즈가 컸음에도 시간 문제로 인해 매번 하드 코딩을 했었는데 이번 기회에 재사용 컴포넌트를 많이 만들어볼 수 있어서 많이 배웠습니다! props로 어떤 컴포넌트인지 나눠주어 필요할 때 props 값을 인자로 내려주는 방법을 알게 되었습니다,, 질문 많이 받아준 승연이와 성희에게 감사를,,
  • 공통 스타일은 emotion css를, 서로 다른 인자를 내려받는 props에는 styled를 먹여보았습니다. 공통 스타일을 css로 먹여주니 코드가 컴팩트해져서 좋았습니다!

리뷰 요구사항 💬

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

  • theme이 아직 확정되지 않아 임의로 지정한 컬러가 많습니다. 참고 부탁드려요!

  • 케이스별로 아이콘 색상이 변경되어야 하는데 stroke 값에 theme을 아무리 적용해도 색상이 바뀌지 않는 문제가 있습니다. 관련하여 피드백 부탁드리겠습니다 ㅜㅜ (이건 승연이가 올려준 pr에 비슷한 부분이 있는 것 같아 pr올리고 뜯어보고 있겠습니다!) > 완료

  • 아이콘을 피그마에서 바로 export 하였는데 구현한 버튼을 보면 icon 크기가 피그마 크기와 맞지 않는 문제점이 있습니다..width 값을 조정하여도 먹히지 않는데 관련하여 이유를 아신다면 피드백 부탁드립니다 ㅜㅜ > 완료

  • 버튼이 disabled인 경우, isDisabled: boolean으로 props를 생성한 뒤, true일 때 스타일을 적용하고, false일 때 default, hover, pressed 스타일을 적용하고 싶습니다. styled 내에서 props에 따른 스타일 속성값을 적용해본 적이 없어서 어려움을 겪고 있는데, 관련하여서도 피드백 부탁드리겠습니다..

  • Status ~~~ btn 에서 Text btn을 재사용하고 있습니다. 하지만 Text btn에서의 default 스타일과, Status ~~ btn에서 사용되는 Text btn의 default 스타일이 다른데, 이럴 경우는 스타일 속성을 어떻게 적용할 수 있을지가 궁금합니다.

  • 레전드 피드백 부탁.. 죄송합니다 ㅜㅜ 코리 달아주시면 최대한 빠르게 적용해서 구현해보겠습니다!!!!1

관련 이슈

close #27

스크린샷 (선택)

2024-07-07.4.54.04.mov

@Kjiw0n Kjiw0n force-pushed the feat/#27/icon-components branch from 56b9dc0 to 69eef94 Compare July 7, 2024 09:18
background-color: ${({ theme }) => theme.palette.WITHE};
border-radius: 6px;

${({ theme }) => theme.fontTheme.CAPTION_01}; /* 수정 필요 */
Copy link
Contributor

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.

하지만 너무너무 많은 이슈,, ㅜ

height: 3.2rem;

background-color: ${({ theme }) => theme.palette.BLUE_DISABLED}; /* 수정 필요 */
border-radius: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

rem으로 바꿔주세요~~

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 26 to 54

const SettingCheck1Layout = styled.button`
display: flex;
align-items: center;
justify-content: center;
width: 2.4rem;
height: 2.4rem;

background-color: ${({ theme }) => theme.palette.BLUE_DISABLED}; /* 수정 필요 */
border-radius: 8px;

&:hover {
background-color: ${({ theme }) => theme.palette.BLUE_DISABLED};
}

&:active {
background-color: ${({ theme }) => theme.palette.BLUE_PASSED};
}
`;

const DoneLayout = styled.button`
display: flex;
align-items: center;
justify-content: center;
width: 3.2rem;
height: 3.2rem;

background-color: ${({ theme }) => theme.palette.BLUE_DISABLED}; /* 수정 필요 */
border-radius: 10px;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const SettingCheck1Layout = styled.button`
display: flex;
align-items: center;
justify-content: center;
width: 2.4rem;
height: 2.4rem;
background-color: ${({ theme }) => theme.palette.BLUE_DISABLED}; /* 수정 필요 */
border-radius: 8px;
&:hover {
background-color: ${({ theme }) => theme.palette.BLUE_DISABLED};
}
&:active {
background-color: ${({ theme }) => theme.palette.BLUE_PASSED};
}
`;
const DoneLayout = styled.button`
display: flex;
align-items: center;
justify-content: center;
width: 3.2rem;
height: 3.2rem;
background-color: ${({ theme }) => theme.palette.BLUE_DISABLED}; /* 수정 필요 */
border-radius: 10px;
const alignCenterStyle = css`
display: flex;
align-items: center;
justify-content: center;
`
const SettingCheck1Layout = styled.button`
${alignCenterStyle}
width: 2.4rem;
height: 2.4rem;
background-color: ${({ theme }) => theme.palette.BLUE_DISABLED}; /* 수정 필요 */
border-radius: 8px;
&:hover {
background-color: ${({ theme }) => theme.palette.BLUE_DISABLED};
}
&:active {
background-color: ${({ theme }) => theme.palette.BLUE_PASSED};
}
`;
const DoneLayout = styled.button`
${alignCenterStyle}
width: 3.2rem;
height: 3.2rem;
background-color: ${({ theme }) => theme.palette.BLUE_DISABLED}; /* 수정 필요 */
border-radius: 10px;

이런 식으로 중복 스타일 빼서 재활용할 수 있을 것 같아요!

Comment on lines 16 to 33
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}
background-color: ${({ theme }) => theme.palette.GREY_01};

&:active {
background-color: ${({ theme }) => theme.palette.GREY_03};
}
`;
Copy link
Member

Choose a reason for hiding this comment

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

여기서 DeleteBtnCss는 왜 나눈 건가욤??

Copy link
Member Author

Choose a reason for hiding this comment

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

props로 컴포넌트 하나로 두개 케이스 재사용하려다가 분리하는게 낫겠다고 판단하면서 빼먹은 것 같습니다 ㅜ!! styled로 통일시켜두겠습니당 ㅎㅎ

Comment on lines +16 to +24
const SettingCheck2Css = css`
display: flex;
align-items: center;
justify-content: center;
width: 2.4rem;
height: 2.4rem;

border-radius: 8px;
`;
Copy link
Member

Choose a reason for hiding this comment

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

SettingCheck3Css 도 같은 구성으로 사용되는 것 같은데,

  1. 파일을 하나 만들어서 export 하거나
  2. 컴포넌트 내 아이콘만 바뀌는 구조
    로 돌려 써도 좋을 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

오 조금 더 컴팩트하게 재사용하는 방법이 있었군요!! 요거 우선 급한 light 케이스 적용부터 해결한 뒤 반영해보도록 하겠습니다!! 자세한 코리 정말 정말 감사합니다 ㅎㅎ 황막리 짱 (황금막내리드 ㅋ)

Comment on lines 16 to 37
const SettingCheck3Css = css`
display: flex;
align-items: center;
justify-content: center;
width: 2.4rem;
height: 2.4rem;

border-radius: 8px;
`;

const SettingCheck3Layout = styled.button`
${SettingCheck3Css}
background-color: ${({ theme }) => theme.palette.BLUE_DISABLED}; /* 수정 필요 */

&:hover {
background-color: ${({ theme }) => theme.palette.BLUE_DISABLED};
}

&:active {
background-color: ${({ theme }) => theme.palette.PRIMARY}; /* svg 색 수정 필요 */
}
`;
Copy link
Member

Choose a reason for hiding this comment

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

세팅버튼 쪽 이부분이 반복되는 것 같은데 여기도 묶어서 한번만 선언해두고 전체 세팅버튼에서 돌려 써도 좋을 것 같아요~~!!


const StatusDoneBtnLayout = styled.div`
display: flex;
gap: 4px;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
gap: 4px;
gap: 0.4rem;

으로 수정하면 화면사이즈 변화에 대응하기 더 수월할 것 같아요~~!

height: 2.4rem;
padding: 0.4rem;

border: 1px solid var(--white, #fff);
Copy link
Member

Choose a reason for hiding this comment

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

색상 쪽 테마 컬러 사용해주면 좋을 것 같아요!! 아직 안 나와서 임시라면 일단 그냥 둬도 괜찮을 것 같슴다~~!!


const StatusInProgressBtnLayout = styled.div`
display: flex;
gap: 4px;
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 17 to 28
const StatusStagingBtnLayout = styled.div`
display: flex;
gap: 4px;
align-items: center;
justify-content: center;
width: 5.2rem;
height: 2.4rem;
padding: 0.4rem;

border: 1px solid var(--white, #fff);
border-radius: 10px;
`;
Copy link
Member

Choose a reason for hiding this comment

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

얘도 StatusTodoBtnLayout 랑 겹쳐서 같은 부분들 다 묶어서 export 하고 재사용하면 좋을 것 같아요!

Comment on lines 24 to 25
/* stylelint-disable-next-line custom-property-pattern */
border-radius: var(--Corner-Extra-large, 28px);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/* stylelint-disable-next-line custom-property-pattern */
border-radius: var(--Corner-Extra-large, 28px);
border-radius: 50%;

로 해줘도 동그랗게 잘 말릴 것 같슴다~~


const TodayPlusBtnCss = css`
display: flex;
gap: 6px;
Copy link
Member

Choose a reason for hiding this comment

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

요기도 rem 부탁드립니당

Copy link
Member Author

Choose a reason for hiding this comment

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

코리 감사합니다!! 짜잘한 부분까지 빠지지 않고 리뷰 달아주셔서 감동입니다,, 🥲 이모지 달은 부분은 전부 반영해두었어요!! 공통되는 스타일 파일 하나 생성해서 재사용하는 부분은 아직 시간 상 적용하지 못했습니다.. 도전해보겠습니다 ;0; 전부 구현 완료했으니 코드 한번 확인해주시면 감사하겠습니다!! (--)(_ _) (인사하는 것임)

@wrryu09
Copy link
Member

wrryu09 commented Jul 7, 2024

버튼이 disabled인 경우, isDisabled: boolean으로 props를 생성한 뒤, true일 때 스타일을 적용하고, false일 때 default, hover, pressed 스타일을 적용하고 싶습니다. styled 내에서 props에 따른 스타일 속성값을 적용해본 적이 없어서 어려움을 겪고 있는데, 관련하여서도 피드백 부탁드리겠습니다..

의 경우에 true인 경우 나와야 하는 스타일과 나머지 경우의 스타일 2가지를 css로 묶어두고, styled.button 에 props로 isDisabled를 받아서 isDisabled 여부에 따라 적용할 css 묶음 적어주면 될 것 같습니당

@jeeminyi jeeminyi merged commit 94b385a into develop Jul 7, 2024
2 checks passed
@wrryu09 wrryu09 deleted the feat/#27/icon-components branch July 10, 2024 09:20
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.

[FEAT] 공통 컴포넌트 아이콘 퍼블리싱
3 participants