-
Notifications
You must be signed in to change notification settings - Fork 117
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
김창민[week14] #972
base: part3-김창민
Are you sure you want to change the base?
김창민[week14] #972
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
템플릿 코드라서 리뷰드리는게 조금 애매한 것 같네요~
input 컴포넌트 위주로 리뷰 남겼습니다!
error: { message: string }; | ||
handleChange: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
} | ||
export default function TextFieldInput({ |
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.
컴포넌트는 왠만하면 파일명이랑 내부의 컴포넌트명이랑 동일하게 가져가는게 좋습니다!
interface Props { | ||
placeholder: string; | ||
disabled: boolean; | ||
error: { message: 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.
추후에 error에 대한 정보가 message 외에 더 필요할수도 있겠지만, 지금 당장 요구사항에서는 아래와 같이 작성해주는게 좋을 것 같네요~
옵셔널도 추가해서요!
error: { message: string }; | |
errorMessage?: string; |
placeholder: string; | ||
disabled: boolean; |
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.
이렇게 2가지 값도 옵셔널하게 받는게 맞을 것 같네요~
사용하는 입장에서 placeholder, disabled 속성을 꼭 사용하진 않을수도 있으니까요!
disabled, | ||
error, | ||
handleChange, | ||
...props |
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 | |
...rest |
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.
근데 현재는 placeholder, disabled, error, handleChange를 제외하고 더 받을 수 있는 값이 없는걸로 보여서 의미가 없는 코드같네요!
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.
아 그러네요. 이게 기존에 styledcomponent로 사용했을 때는 저 ...props를 받아서 return 부분에 입력하면 다른 페이지에서 input 공용 컴포넌트를 불러온 뒤에 css 스타일을 마음대로 추가 및 변경할 수 있어서 그렇게 사용했습니다.
혹시 rest로 바꿔도 동일하게 사용이 가능할까요?
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의 타입에서 추가적으로 값을 받을 수 있어야 나머지 매개변수(...rest)가 의미가 있을 것 같아요!
interface InputProps extends ComponentPropsWithoutRef<'input'> {
...
}
handleChange, | ||
...props | ||
}: Props) { | ||
const [status, setStatus] = useState("inActive"); |
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.
이런 경우에는 status라는 상태에 올 수 있는 값이 정해져 있기 때문에 유니온 타입으로 타입을 좁혀보면 좋을 것 같네요~
type Status = 'inActive' | 'focused' | ...
const [status, setStatus] = useState<Status>("inActive");
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 [status, setStatus] = useState("inActive"); | |
const [status, setStatus] = useState("inactive"); |
export const FolderButton = ({ text, onClick, isSelected = false }: FolderButtonProps) => { | ||
return ( | ||
<button className={cx("container", { selected: isSelected })} onClick={onClick}> | ||
<span>{text}</span> |
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.
children으로 표현되는게 좋겠네요~
const cx = classNames.bind(styles); | ||
|
||
type FolderInfoProps = { | ||
profileImage: 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.
img의 src 속성에 넣어줄 값이라 이렇게 표현되면 좋겠네요!
profileImage: string; | |
profileImageUrl: string; |
@@ -0,0 +1,5 @@ | |||
import { SharedPage } from "../component/SharedPage.tsx"; |
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.
import할때 확장자는 붙여주지 않아도 됩니다~
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.
파일명이 이상하네요~
if (onBackdropClick) { | ||
onBackdropClick(event); | ||
} |
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.
이런식으로 써주면 됩니다~
if (onBackdropClick) { | |
onBackdropClick(event); | |
} | |
onBackdropClick?.(event); |
머지를 위해서 .gitignore쪽 충돌만 수정해주시면 좋을 것 같습니다~ |
요구사항
기본
심화
주요 변경사항
프로젝트 과제중에 제가 작성했던 기존의 input 컴포넌트를 수정해서 사용했습니다.
스크린샷
멘토에게