-
Notifications
You must be signed in to change notification settings - Fork 39
[문주영] Sprint10 #228
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
[문주영] Sprint10 #228
The head ref may contain hidden characters: "Next-\uBB38\uC8FC\uC601-sprint10"
Conversation
GANGYIKIM
left a comment
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.
주영님 10번째 미션 수고하셨습니다!
공부도 하시면서 꾸준히 스프린트 미션 하시는 모습이 너무 대단합니다.
다음 미션도 화이팅입니다!
global.d.ts를 이렇게 선언하는 게 맞을까요?
d.ts는 definition 파일 혹은 declare 파일이라고 합니다. 타입스크립트에서 타입 추론을 도와주기위해 사용자가 제공하는 타입 정의 파일입니다. 지금이 경우 svg와 module.css의 타입추론을 도와주기 위해 d.ts 파일을 선언해야 하지만 지금의 타입정보는 부족합니다.
declare module "*.svg"는 .svg로 끝나는 확장자 파일을 import 할 수 있게 해주지만 더 자세한 정보는 없기 때문에 any로 추론됩니다.
지금처럼 svg, module.css를 사용하실 경우는 아래처럼 작성하시면 됩니다.
(svg를 webpack/svgr을 사용해 컴포넌트처럼 사용하실 경우 해당 문서를 따라하시면 됩니다.)
declare module "*.module.css";
declare module "*.svg" {
const value: string;
export default value;
}
그리고 타입스크립트가 해당 d.ts 파일을 인식할 수 있도록 tsconfig 파일의 include 에 추가해주시면 됩니다.
| type LinkAttributes = AnchorHTMLAttributes<HTMLAnchorElement> & LinkProps; | ||
|
|
||
| interface Props extends LinkAttributes { | ||
| href: 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.
💊 제안
href 속성이 LinkProps에서 필수값으로 선언되어 있으므로 href 부분이 중복됩니다. 지우셔도 될 것 같아요.
| href: 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.
💊 제안
CheckList라는 컴포넌트 명은 "List" 때문에 부적절한 것 같아요. CheckItem 같은 컴포넌트명을 추천드립니다.
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.
💊 제안
요구사항으로 인해 어쩔 수 없지만 a 태그 안에 button 태그가 들어가 있어 구조가 명확하지 않습니다.
이런 경우 CheckList 컴포넌트를 아래처럼 작성하셔서 조금 더 구조를 명확하게 작성하시는 것을 추천드립니다.
export default function CheckItem({
className = "",
children,
isChecked,
...props
}: Props) {
return (
<Link
className={`${styles["check-list"]} ${
styles[String(isChecked)]
} ${className}`}
{...props}
>
{children}
</Link>
);
}<CheckItem>
<button type="button" onClick={onClick}>
<Image alt="checkbox" src={isChecked ? ic_checked : ic_empty} />
</button>
{item.name}
</CheckItem>또한 button 클릭 시 링크 이동이 되지 않도록 Event.preventDefault 함수를 호출하세요~
| interface Props extends HTMLAttributes<HTMLDivElement> { | ||
| defaultIsChecked: boolean; | ||
| defaultValue: string; | ||
| } | ||
|
|
||
| export default function CheckListDetail({ | ||
| className = "", | ||
| defaultIsChecked = false, | ||
| defaultValue = "", | ||
| ...props | ||
| }: Props) { | ||
| const [value, setValue] = useState(defaultValue ?? ""); | ||
| const [isChecked, setIsChecked] = useState(defaultIsChecked); |
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.
💊 제안
defaultValue, defaultIsChecked와 같이 변수명을 다르게 지정하신 이유가 props와 내부 상태 변수명이 충돌하거나 혼동될 수 있어서라면, 아래와 같이 구조 분해 할당 시 별칭을 주는 방식으로 작성하셔도 됩니다!
| interface Props extends HTMLAttributes<HTMLDivElement> { | |
| defaultIsChecked: boolean; | |
| defaultValue: string; | |
| } | |
| export default function CheckListDetail({ | |
| className = "", | |
| defaultIsChecked = false, | |
| defaultValue = "", | |
| ...props | |
| }: Props) { | |
| const [value, setValue] = useState(defaultValue ?? ""); | |
| const [isChecked, setIsChecked] = useState(defaultIsChecked); | |
| interface Props extends HTMLAttributes<HTMLDivElement> { | |
| isChecked: boolean; | |
| value: string; | |
| } | |
| export default function CheckListDetail({ | |
| className = "", | |
| isChecked:_isChecked= false, | |
| value:_value = "", | |
| ...props | |
| }: Props) { | |
| const [value, setValue] = useState(_value ?? ""); | |
| const [isChecked, setIsChecked] = useState(_isChecked); |
| useEffect(() => { | ||
| const width = (spanRef.current?.offsetWidth ?? 0) + 1; | ||
| if (inputRef.current) inputRef.current.style.width = width + "px"; | ||
| }, [value]); | ||
|
|
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.
❗️ 수정요청
디자인을 봤을 때 CSS로 width를 조작하셔도 될 것 같아요. js로 width를 조작해야할 이유가 없다면, css로 처리를 해주세요.
지금의 경우 span 요소의 offsetWidth 에 따라 input 의 width가 결정되도록 되어 있어 작성한 할 일이 제대로 보이지 않습니다.
확인해보시고 수정하시면 좋겠습니다. 또한 할 일의 글자수가 길 경우도 고려해서 수정해보시면 더 좋겠습니다.
| <button type="button" onClick={() => setIsChecked((prev) => !prev)}> | ||
| <Image alt="checkbox" src={isChecked ? ic_checked : ic_empty} /> | ||
| <input type="hidden" name="isCompleted" value={String(isChecked)} /> | ||
| </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.
💊 제안
버튼 태그안에 해당 input이 있어야 하는 이유를 모르겠습니다~ 상호작용되거나 form으로 해당 input의 값이 읽혀 제출될 필요가 없다면 지우시는 것을 추천드려요.
| const formRef = useRef<HTMLFormElement>(null); | ||
| const router = useRouter(); | ||
|
|
||
| const handleInputChange = async (e: 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.
💊 제안
swagger에 명시된 업로드 가능한 파일의 조건은 5MB이하의 이미지 파일입니다. file input에서 accept로는 확장자를 제안해줄 뿐이니 해당 함수에서 파일의 확장자와 크기를 확인하고 조건에 따라 처리하는 로직을 추가하시면 좋겠습니다.
| alert((error as Error).message); | ||
| setDisabled(false); |
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.
💊 제안
타입 단언문대신 아래처럼 타입가드를 통해 작성하시는 것을 추천드립니다.
| alert((error as Error).message); | |
| setDisabled(false); | |
| if (error instanceof Error) { | |
| alert(error.message); | |
| } | |
| setDisabled(false); |
| const area = document.querySelector<HTMLTextAreaElement>( | ||
| "form label textarea" | ||
| ); | ||
| if (area) area.style.height = Math.min(area.scrollHeight, 229) + "px"; |
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.
💊 제안
해당 로직이 중복되니 함수로 빼시면 더 좋겠습니다.
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게
global.d.ts를 이렇게 선언하는 게 맞을까요?