Skip to content

Conversation

@Woolegend
Copy link
Collaborator

@Woolegend Woolegend commented Nov 16, 2024

요구사항

  • Github에 PR(Pull Request)을 만들어서 미션을 제출합니다.
  • 피그마 디자인에 맞게 페이지를 만들어 주세요.
  • Typescript를 사용합니다

기본

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

심화

  • any타입을 최소한으로 써주세요

멘토에게

  • 상품 등록 페이지에서 등록된 이미지를 삭제할 때 오류가 발생합니다. 기능적인 문제는 없지만 해결이 되는 대로 push하겠습니다.
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@Woolegend Woolegend added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다.. labels Nov 16, 2024
@Woolegend Woolegend self-assigned this Nov 17, 2024
@Woolegend Woolegend removed the 미완성🫠 죄송합니다.. label Nov 17, 2024
@Woolegend Woolegend requested review from kiJu2 and removed request for arthurkimdev November 18, 2024 09:00
@kiJu2
Copy link
Collaborator

kiJu2 commented Nov 18, 2024

수고 하셨습니다 ! 스프리트 미션 하시느라 정말 수고 많으셨어요.
학습에 도움 되실 수 있게 꼼꼼히 리뷰 하도록 해보겠습니다. 😊

Comment on lines +5 to +9
<<<<<<< HEAD
=======
# See https://help.github.com/articles/ignoring-files/ for more about ignoring files.

>>>>>>> cde515c40685c7ea849fcafc91354005f21c13fd
Copy link
Collaborator

Choose a reason for hiding this comment

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

컨플릭트 해결 중 누락된 건이 있는 것 같아요 !

Comment on lines +67 to +79
<<<<<<< HEAD
# End of https://www.toptal.com/developers/gitignore/api/react,nextjs,sass
=======
# End of https://www.toptal.com/developers/gitignore/api/react,nextjs,sass
.env.local
.env.development.local
.env.test.local
.env.production.local

npm-debug.log*
yarn-debug.log*
yarn-error.log*
>>>>>>> cde515c40685c7ea849fcafc91354005f21c13fd
Copy link
Collaborator

Choose a reason for hiding this comment

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

컨플릭트 해결 중 누락된 건이 있는 것 같아요 !

page: number = 1,
pageSize: number = 12,
orderBy: string = "recent",
keyword: string | undefined = undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

다음과 같이 옵셔널 프로퍼티로 작성할 수 있습니다 ! 😊

Suggested change
keyword: string | undefined = undefined
keyword?: string = undefined

export async function getProductList(
page: number = 1,
pageSize: number = 12,
orderBy: string = "recent",
Copy link
Collaborator

Choose a reason for hiding this comment

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

타입 범위를 좁힐 수 있을 것 같아요 ! 😊

Suggested change
orderBy: string = "recent",
orderBy: "recent" | "liked" = "recent",


const BASE_URL = process.env.REACT_APP_API_BASE_URL;

export async function getProductList(
Copy link
Collaborator

Choose a reason for hiding this comment

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

기본값 세팅도 정말 잘 되어있네요 ㅎㅎㅎ 👍

const handleFeatureClick = (e) => {
if (e.target.classList.contains("feature")) {
setSelectedComment(e.currentTarget.dataset.id);
const [comments, setComments] = useState<CommentList | undefined>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

기본값이 빈 값으로 되어 있다면 자동으로 undefined이 추론됩니다 !

Suggested change
const [comments, setComments] = useState<CommentList | undefined>();
const [comments, setComments] = useState<CommentList>();

Comment on lines +27 to +29
const [selectedComment, setSelectedComment] = useState<string | null>(null);
const [isEditing, setIsEditing] = useState<string | null>(null);
const selectRef = useRef<HTMLUListElement | 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을 고려한 타입을 잘 설정하셨군요 ㅎㅎㅎ 👍

Suggested change
const [selectedComment, setSelectedComment] = useState<string | null>(null);
const [isEditing, setIsEditing] = useState<string | null>(null);
const selectRef = useRef<HTMLUListElement | null>(null);
const [selectedComment, setSelectedComment] = useState<string>(null);
const [isEditing, setIsEditing] = useState<string>(null);
const selectRef = useRef<HTMLUListElement | null>(null);

Comment on lines +59 to +61
alert(
`test : comment id ${event.currentTarget.dataset.id} 삭제 되었습니다`
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

사용자 알림도 너무 좋아요 ㅎㅎㅎ

Comment on lines +141 to +143
isSelected: any;
onClick: any;
onSelect: any;
Copy link
Collaborator

Choose a reason for hiding this comment

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

다음 미션 때에는 다음과 유사하게 정의되도록 해봅시다 !

Suggested change
isSelected: any;
onClick: any;
onSelect: any;
isSelected: boolean;
onClick: (selectedId) => void;
onSelect: (selectedId) => void;

위에 작성된 코드는 검증되지 않은 타입입니다 ! 한번 확인해봐주세요 ㅎㅎㅎ 관건은 any를 쓰지 말자 ! 로 보시면 될 것같아요.

아마도, 어떤 원인이 있어서 일단은 any로 두신 것 같아요 ! 차근차근 지워봅시다 !

import useDebounce from "../hooks/useDebounce";

const DeviceTypeContext = createContext();
export type DeviceType = "desktop" | "tablet" | "mobile";
Copy link
Collaborator

Choose a reason for hiding this comment

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

유니온 타입을 적절하게 설정하셨군요 ! 훌륭합니다 ㅎㅎㅎ

@kiJu2
Copy link
Collaborator

kiJu2 commented Nov 18, 2024

수고 많으셨습니다 재현님 !
열심히 개발에 몰입하시고, 학습에 대한 의욕. 원활한 커뮤니케이션과 객관적인 시각을 갖추셨어요.
리스펙 드리며, 분명 빠른 시일 내에 멋진 개발자가 되실거라고 생각합니다 ㅎㅎ

그 간 정말 고생 많으셨고 앞으로도 승승장구하시길 진심으로 기원합니다 ! 🙇‍♂️🙇‍♂️

@kiJu2 kiJu2 merged commit 322d2ec into codeit-bootcamp-frontend:React-우재현 Nov 18, 2024
@Woolegend
Copy link
Collaborator Author

Woolegend commented Nov 18, 2024

조건 분기를 통해 여러 경우를 한 번에 처리하는 함수가 많습니다.

AddItemPage.tsx에 있는 handleChange, handleDelete가 이에 해당됩니다.

나름 잘 만든 함수라고 생각했었는데 TS로 마이그레이션 해보니 엉망인 거 같습니다.

가장 문제가 되는 건 prev[name]에 올 수 있는 타입이 너무 많다 보니 any를 쓰는 것과 다를 바 없는 거 같습니다.

변수의 타입이 불명확하다 보니 필요할 때 마다 타입을 명시 해줘야 하고, 이로 인해 코드 또한 효율성과 가시성이 많이 떨어 져 보입니다.

그래서 이러한 부분을 개선해 아래처럼 바꿔보려고 하는데 멘토님의 고견을 듣고 싶습니다!

  1. 변환할 값의 타입에 따라 핸들러를 만듭니다. handleChange<타입>
  2. 슈퍼 핸들러를 만들어 값의 타입에 따라 알맞은 타입의 핸들러로 보내줍니다. handleChange
const handleChangeObject = (e) => { ... }
const handleChangeArray = (e) => { ... }
const handleChangeString = (e) => { ... }

const handleChange = (e) => {
  if(객체일 ) handleChangeObject(e);
  if(배열일 ) handleChangeArray(e);
  if(문자열일 ) handleChangeString(e);
  ...
}

@kiJu2
Copy link
Collaborator

kiJu2 commented Nov 18, 2024

음 함수의 책임이 많아짐에따라 단일 책임원칙이 위배될 수 있고, 여러 유니온 타입 혹은 오버로딩에 의해 타입 안전성이 저하될 수 있다고 판단되기에 각각의 상태 별로 핸들러를 작성했을 것 같아요.
다만, 이렇게 한다면 코드가 상당히 길어질 수 있겠죠 ?
따라서, 커스텀 훅(use-add-item-form)을 만들어서 관리하면 어떨까 싶습니다 ! 😊

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