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 #239

Conversation

Jaeheon96
Copy link
Collaborator

@Jaeheon96 Jaeheon96 commented Aug 2, 2024

요구사항

기본

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

심화

심화 요구사항 없음

주요 변경사항

  • React 시작하며 마이그레이션하지 못했던 랜딩페이지와 로그인, 회원가입 페이지 구현
  • netlify 배포

멘토에게

  • 로그인과 회원가입 페이지의 유효성 검사를 위한 onChange를 각 input이 아니라 부모 form에 달아줬더니 input에 value 프로퍼티가 있으면 onChange 없이 value를 주고 있다는 경고를 뱉어냅니다. 일단 콘솔창의 경고를 해결하기 위해 value 프로퍼티를 지웠습니다. 코드잇 강의에서 별다른 이유가 없으면 input에 value 프로퍼티를 줘서 controlled input으로 만들기를 권장한다고 해서 value 프로퍼티를 줬던건데, 이런 경우에는 value 프로퍼티가 없는게 맞는걸까요?
  • netlify 배포 주소: https://sprintpandamarket.netlify.app/
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@Jaeheon96 Jaeheon96 added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Aug 2, 2024
@Jaeheon96 Jaeheon96 self-assigned this Aug 2, 2024
@Jaeheon96 Jaeheon96 requested a review from jyh0521 August 2, 2024 10:13
Copy link
Collaborator

@jyh0521 jyh0521 left a comment

Choose a reason for hiding this comment

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

과제하느라 고생하셨습니다! 추가로 웹 개발에서 파일 내에 탭 사이즈는 2로 해주시면 좋을 것 같습니다.

Comment on lines +1 to +6
export enum SignupFormProps {
email = "email",
nickname = "nickname",
password = "password",
verifyPassword = "verifyPassword",
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

enum 활용해보신 점 좋네요!

Comment on lines +2 to +12
createdAt: string;
description: string;
favoriteCount: number;
id: number;
images: string[];
isFavorite: boolean;
name: string;
ownerId: number;
price: number;
tags: string[];
updatedAt: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

타입을 선언해주실때, 어느정도 순서를 생각해서 작성해주셔도 좋습니다. id 같은 값이 최상위로 오고 createdAt이나 updatedAt 같은건 묶어주시는 등의 방식으로요!

@@ -25,18 +33,19 @@ function AddItemFormHeader({ values }) {
)
}

function FileInputSection({ onChange, valuesImages }) {
function FileInputSection({ onChange, valuesImages }: { onChange: (name: string, value: any) => void, valuesImages: string | null}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

컴포넌트의 props 타입은 따로 선언해서 사용해주셔도 좋습니다!


const [preview, setPrieview] = useState(null);
const [preview, setPrieview] = useState<string | null>(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

null 사용도 좋지만 undefined를 활용하시면 null을 사용하지 않고 타입 지정이 가능합니다.


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.

ref를 사용하지 않고 구현하는 방법도 알고 계시면 좋을 것 같습니다.

@@ -0,0 +1,30 @@
.LoginShortcutContainer {
width: 100%;
height: 4.625rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

rem값도 px로 변환했을때 소수점이 안나오게 작성해주시는 것을 추천드립니다.

height: 4.625rem;
border-radius: 0.5rem;
padding: 1rem 1.5rem;
background-color: #E6F2FF;
Copy link
Collaborator

Choose a reason for hiding this comment

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

컬러 변수 활용해주시면 좋을 것 같습니다.


interface Props {
name: string;
value: string | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 밑에 작성하신 것 처럼 value?: string 과 같이 작성할 수 있을 것 같아요!

@@ -4,8 +4,9 @@ import Card from './Card';
import './BestSection.css';
import useAsync from '../../hooks/useAsync';
Copy link
Collaborator

Choose a reason for hiding this comment

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

alias path 사용해보시는 것을 추천드립니다.

verifyPasswordError: '',
}

function useLoginFormChange(formValues: SignupFormType, setFormValues: React.Dispatch<SetStateAction<SignupFormType>>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

훅에서 formValues를 받아오게 하지 않고 훅에서 선언해줘서 외부로 return 해주는 방법도 있습니다!

@jyh0521 jyh0521 merged commit 0b997c4 into codeit-bootcamp-frontend:React-손재헌 Aug 3, 2024
@jyh0521
Copy link
Collaborator

jyh0521 commented Aug 3, 2024

로그인과 회원가입 페이지의 유효성 검사를 위한 onChange를 각 input이 아니라 부모 form에 달아줬더니 input에 value 프로퍼티가 있으면 onChange 없이 value를 주고 있다는 경고를 뱉어냅니다. 일단 콘솔창의 경고를 해결하기 위해 value 프로퍼티를 지웠습니다. 코드잇 강의에서 별다른 이유가 없으면 input에 value 프로퍼티를 줘서 controlled input으로 만들기를 권장한다고 해서 value 프로퍼티를 줬던건데, 이런 경우에는 value 프로퍼티가 없는게 맞는걸까요?

form 말고 각 input에다가 onInput 핸들러 함수를 작성해주시고 각 input에 value 사용해보실래요?

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.

2 participants