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

[이경원] sprint8 #518

Open
wants to merge 35 commits into
base: React-이경원
Choose a base branch
from

Conversation

gracelee5
Copy link
Collaborator

@gracelee5 gracelee5 commented May 23, 2024

요구사항

기본

  • 스프린트 미션 1 ~ 7에 대해 typescript를 적용해주세요

주요 변경사항

  • typescript로 변경

스크린샷

멘토에게

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

hanseulhee and others added 30 commits October 10, 2023 14:15
…ithub-actions

[Fix] delete merged branch github action
…rint-Mission into React-이경원-sprint5

로컬 브랜치와 원격 브랜치 병합합
@gracelee5 gracelee5 requested a review from domuk-k May 23, 2024 16:14
@gracelee5 gracelee5 self-assigned this May 23, 2024
@gracelee5 gracelee5 added the 순한맛🐑 마음이 많이 여립니다.. label May 23, 2024
@gracelee5 gracelee5 changed the title React 이경원 sprint8 [이경원] sprint8 May 23, 2024
Copy link
Collaborator

@domuk-k domuk-k left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 👍
package.json에 타입체크하는 스크립트를 추가하셔서 활용해보시면 좋겠습니다.
타입깨지는 부분없이 잘 수행하셨네요!

"scripts": {
"tsc": "tsc --noEmit"
}

}

function CommentInput(): JSX.Element {
const [comment, setComment] = useState<string>("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

제네릭을 활용하셨네요 👍
다만 initialState로 "" 을 전달하셨기 때문에, 제네릭없이도 string으로 잘 추론됩니다. 그걸 활용하시는 편이 좋습니다.

  • useState의 타입정의를 보면, 첫 인자의 타입이 추론되게 하여 편하게 사용할 수 있게 하려는 의도를 엿볼 수 있습니다. (첫 인수인 initialState의 타입으로 타입S를 추론)
    function useState<S>(initialState: S | (() => S)): [S, Dispatch<SetStateAction<S>>];
  • 타입추론이 잘된다면 명시하지않는게 TS를 효과적으로 활용하는 방법이라고 알려져있습니다 - 이펙티브타입스크립트

function CommentInput(): JSX.Element {
const [comment, setComment] = useState<string>("");

const handleInputChange = (event: ChangeEvent<HTMLTextAreaElement>): void => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍
참고로, 이렇게 하셔도 간결하겠네요. 함수를 참조하는 변수 handleInputChange 자체에 타입을 지정해줄 수 있습니다

  const handleInputChange: ChangeEventHandler<HTMLTextAreaElement> = (event) => {
    setComment(event.target.value);
  };

@@ -65,7 +70,7 @@ const InputArea = styled.textarea`
}
}
`;
const RegisterButton = styled.button`
const RegisterButton = styled.button<RegisterButtonProps>`
Copy link
Collaborator

Choose a reason for hiding this comment

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

A컴포넌트 구현에 바로위애 A컴포넌트의 인터페이스가 작성되는게 자연스러운 패턴으로 알고 있습니다
RegisterButtonProps 를 RegisterButton 위 라인에 두는 걸 제안드려요

function CommentList() {
const [comments, setComments] = useState(null);
const [comments, setComments] = useState<Comment[]>([]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

const [preview, setPreview] = useState(initialPreview);
const inputRef = useRef();
const inputRef = useRef<HTMLInputElement>(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

read-only로 안전하게 사용하셨군요.

useRef는 첫 인자를 null로 전달하면 read-only가 되는 특징이 있습니다. useRef를 cmd를 누른채로 클릭해서 타입선언을 확인해보세요 😄

2024-05-26.10.46.46.mov

const validateInputs = useCallback(() => {
const isValid =
values.title.trim() !== "" &&
values.content.trim() !== "" &&
values.price.trim() !== "" &&
values.tag.trim() !== "";
tags.length > 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

tags.trim().length로 하셔야 동작이 유지되겠군요

<Text>판매 가격</Text>
<Price
type="number"
name="price"
value={values.price}
onChange={handleInputChange}
placeholder="판매 가격을 입력해주세요."
></Price>
/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -108,6 +125,7 @@ function AddItem() {
</>
);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -54,7 +69,7 @@ const InputTagContainer = styled.div`
align-items: flex-start;
`;

const TagInput = styled.input`
const InputTag = styled.input`
Copy link
Collaborator

Choose a reason for hiding this comment

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

기존 네이밍이 더 좋아보여요. 이 자체로는 특별한 기능없는 표준 Input 요소이고, Tag(도메인) 를 입력하기 위한 특별한 스타일을 가진 input 요소니까, TagInput이 더 좋아보입니다. export되고 있지않고 이 파일안에서만 사용하고, 이미 "Tag"라는 맥락이 충분히 이 파일안에서 파악가능하니까 그냥 Input이라고만 해도 무방할거같고요. (사실 그러면 공통컴포넌트같아지는 이슈가 있지만...)

Comment on lines +13 to +21
interface Product {
id: number;
name: string;
images: string;
price: number;
description: string;
tags: string[];
favoriteCount: number;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Products 컴포넌트에 있는 Product 인터페이스랑 이름은 같고 내용은 조금다르네요. ./src/types.ts 또는 ./src/types/product.ts 파일에 통합해서 사용하시는것도 방법일거같아요.

여기는 "ProductDetail" 이니까, 다루고자하는 값이 목록과는 맥락이 다를 수 있으니, 이름에 Detail을 붙이시는 편이 낫겠네요.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
순한맛🐑 마음이 많이 여립니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants