-
Notifications
You must be signed in to change notification settings - Fork 1
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] 공통 컴포넌트 컨트롤버튼 퍼블리싱 #293
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 additionalCss 좋은 것 같아요!
styled(컴포넌트) 스타일링 방식은 styled-components 방식이라고 하더라고용 저흰 css커스텀이 장점인 emotion 사용하니까 css 전달으로 스타일링 하는 게 좋아보여요!!
토글 관련 컴포넌트에 몇가지 의견 제안드렸으니 확인부탁드립니다~~
진짜 빠르네요 👍 👍 최고
onClick: () => void; | ||
}; | ||
|
||
function CheckButton({ label, size, checked, onClick }: CheckButtonProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
초깃값...!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가했습니다앗 ㅎ.ㅎ!!!
onClick: () => void; | ||
}; | ||
|
||
function ToggleLabelButton({ active, firstLabel, secondLabel, onClick }: ToggleLabelButtonProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
props 중 active
, onClcik
을 외부에서 전달받는 이유가 있을까요??
어차피 토글의 특성 상 onClick
은 클릭 시 active
를 바꾸는 처리를 담당할텐데 이런 동작은 외부에서 처리하기보다 내부에서 처리하는 것이 추후 컴포넌트 사용 시 더 편리할 듯 합니다!! props도 label만 받도록 적어지니 더 깔끔해질 듯 해서 제안드려용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
흠 버튼 상태에 따라서 표시해야하는 값이 달라져 외부에서 관리하게 될 것 같아서 일단 props로 받도록 만들어뒀었습니다! 그래서 상위 컴포넌트에서 상태를 관리하고 onClick 에 핸들러를 넘겨주어 해당 상태를 컨트롤할 수 있게 만들려고 했는데, 여기에서 state를 일단 만들고 불린값을 뒤집는 핸들러를 onClick 에 넣어주는 게 추후 사용하기에 더 좋을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아하 토글이 전체 페이지를 컨트롤하는 역할을 하네요.. 그럼 기존처럼 props로 받는 것이 좋아보여요!!
페이지 내부 대부분 컴포넌트들의 간격 등도 조정되니 일단 이렇게 두고 필요하면 전역으로 관리하는 방식으로 진행하면 좋을 것 같습니당!!
onClick: () => void; | ||
}; | ||
|
||
function ToggleSwitchButton({ active, onClick }: ToggleSwitchButtonProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에 ToggleLabelButton과 동일하게 active
, onClick
을 내부에서 처리해도 괜찮겠다는 생각이 들어서, 혹시 외부에서 처리해야하는 경우(특별한 경우)가 없다면 내부에서 처리하도록 수정해도 좋을 것 같아요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
나중에 사용할 것까지 고려해서 잘 구성해주신 것 같습니다!! 고생 많으셨습니다 👍
|
||
type Story = StoryObj<typeof meta>; | ||
|
||
export const CheckedLargeButton: Story = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스토리북 작성도 왕 깔끔하네요
@@ -0,0 +1,18 @@ | |||
import { css } from '@emotion/react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shadow스타일을 따로 theme으로 정리하니 공통적으로 사용할 수 있고 좋은 것 같습니다.!
작업 내용 🧑💻
알게된 점 🚀
Button
컴포넌트를 사용하는데,Button
은width
가 따로 지정되어 있지 않고flex
로 설정되어 있어 완료버튼 혼자 사이즈가 달라 설정해주어야 했습니다. + 눌렸을 때 shadow 도 줘야 해서..styled(Button)
처럼 작성하려고 했는데Button
자체는 styled 를 사용한 컴포넌트를 사용하지만.. 리턴하는 건react component
이기 때문에 추가 css가 올라가지 않았습니다. (이거때매styled
구문을 함수 외부로 빼서 props 처리도 해봤는데 되진 않았습니다)Button
의 css를 달리해서 사용할 경우도 많을 것 같아..additionalCss
props를 뚫어서 styled 컴포넌트 마지막 부분에 변수를 넣어주어 기존 css 를 덮어써 적용되도록 만들었습니다.리뷰 요구사항 💬
관련 이슈
close #291
스크린샷 (선택)