-
Notifications
You must be signed in to change notification settings - Fork 4
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
Jy/XButton, Input, Search Filter 구현 #97
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.
전체적으로 잘 작성하신 것 같습니다.
SENCONDARY: '#586069', | ||
SECONDARY: '#586069', |
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.
이런 미세한 오타를 발견해주시다니 👍👍👍
|
||
cursor: pointer; |
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.
속성끼리는 붙이고 & > 같은 다른 스타일 적용할 때만 띄워주면 좋을 것 같아요
const StyledUl = styled.ul``; | ||
const StyledLi = styled.li``; | ||
const StyledButton = styled.button``; |
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.
비어있는 컴포넌트라면 굳이 styled를 사용할 필요 없이 <ul></ul>
처럼 직접 태그를 사용해도 됩니다
<svg | ||
xmlns="http://www.w3.org/2000/svg" | ||
version="1.1" | ||
viewBox="0 0 1000 1250" | ||
x="0px" | ||
y="0px" | ||
> | ||
<g> | ||
<path | ||
className="fil0" | ||
d="M486 761l-400 -501c-6,-8 -5,-18 3,-24 3,-3 7,-4 10,-4l802 0c9,0 17,8 17,17 0,5 -2,9 -5,12l-400 501c-6,7 -16,8 -24,2 -1,-1 -2,-2 -3,-3z" | ||
/> | ||
</g> | ||
</svg> |
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.
svg에 필요없는 속성(x, y, className 등)은 제거하고 넣는게 더 조아용😊😊
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 30" x="0px" y="0px"> | ||
<title>x, decline, reject, cancel, delete</title> | ||
<path d="M19.707,4.293a1,1,0,0,0-1.414,0L12,10.586,5.707,4.293A1,1,0,0,0,4.293,5.707L10.586,12,4.293,18.293a1,1,0,1,0,1.414,1.414L12,13.414l6.293,6.293a1,1,0,0,0,1.414-1.414L13.414,12l6.293-6.293A1,1,0,0,0,19.707,4.293Z" /> | ||
</svg> |
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.
여기도 마찬가지로 svg에 필요없는 속성은 빼주는게 죠습니다.
실제로 속성 하나씩 빼보면서 필요한 속성이 뭔지 눈으로 확인해 보는 것도 좋을 것 같아요 :)
`; | ||
|
||
const Input: FunctionComponent<Props> = ({ | ||
placeholder, |
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 함수, map 활용 등 사용하지 않았던 부분을 구현해주어서 리뷰하며 많이 배워갑니다~!
const Icon = Reset; | ||
const onClick = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
// 필터 초기화 | ||
}; |
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에 어떤 메소드가 들어갈지 적어준 부분 너무 좋아요~!
interface Props { | ||
Icon: FunctionComponent; | ||
value: string; | ||
onClick: (e: React.MouseEvent<HTMLButtonElement>) => void; |
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에서는 event 객체를 활용할 일이 없을 것으로 생각됩니다!
event 객체를 넣어주면 이 컴포넌트를 사용할 때 항상 저 event 객체를 넣어주어야 해서 타입 오류가 잦게 날 수 있을 것 같습니다.
}) => { | ||
return ( | ||
<details> | ||
<StyledSummary> |
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.
summary tag 새롭게 배워갑니다~!
- svg 파일에서 필요 없는 태그 삭제
- props value -> content로 이름 변경 - onClick method 인자 변경 - 버튼 하위 태그 flex로 수평 정렬 - 배경색 투명하게 변경 - css 단위 변경, 벼누 사용 - svg 경로 오타 수정 - component에 맞게 stories 수정
- 필수 값 조정 - css 단위 변경 - 옵션 값 default value 설정
- search filter modal button 생성 - search filter modal 생성
- search filter molecules -> organisms로 변경
input?: string; | ||
type: string; | ||
keys: string[]; | ||
display: string; |
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.
display type을 string보다는 좀더 명확하게 (ex 'flex' | 'block') 하는게 좋습니당
#66 #82 #69