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

[김세환] sprint 8 #246

Conversation

kimsayhi
Copy link
Collaborator

@kimsayhi kimsayhi commented Aug 2, 2024

이번 파트에 들어오면서 타입스크립트를 처음 시작했는데 아직 React도 잘 다루지 못하는것같아
다시 처음부터 만들며 이것저것 한꺼번에 많이 시도해봤습니다. 당연히 useState와 useEffect훅도 잘 못다뤘기에
엄청 오류도 많이나고, 논리를 떠올린다음 작성하는것이 아니라 디버그 찍어가며 아무거나 넣어보면서 해결했기에
제가봐도 어떻게 동작하는지 모르겠을 만큼 로직이 복잡하고 꼬이게 되었습니다.
폴더구조도 정말 난잡하고 어디서 주워듣고 그럴싸해보이게 하려다보니 오히려 복잡해진것같습니다.
앞으로 어떻게 학습해야할지 조금 갈피를 잃었는데 조언해주시면 감사하겠습니다.

현재는 다시 차근차근 해보기위해 해당 커밋의 커스텀훅과 useReducer은 모두지웠습니다!

@kimsayhi kimsayhi force-pushed the React-김세환-sprint8 branch from 06cc149 to 576e0c5 Compare August 2, 2024 13:36
@kimsayhi kimsayhi closed this Aug 2, 2024
@kimsayhi kimsayhi reopened this Aug 2, 2024
@kimsayhi kimsayhi added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다.. labels Aug 2, 2024
@kimsayhi kimsayhi requested a review from Taero-Kim August 2, 2024 14:15
@Taero-Kim Taero-Kim self-assigned this Aug 3, 2024
declare module "*.png" {
const content: string;
export default content;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

이미지 파일을 모듈로 인식하도록 잘 설정하셨네요!
이렇게 하면 이미지를 모듈로 임포트하여 직관적인 사용이 가능해 좋은 방식이에요!

pageSize = 10,
orderBy = "recent",
keyword = null,
}: GetItemListArg) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3;
api에서 응답받은 결과와 함수의 반환타입을 지정하면 더 좋을 것 같아요!
�그래야, 타입 안정성을 더 강화하고, 다른 동료들과 협업할 때 코드에 대한 이해를 하기 더 쉬울 것 같아요!

ex)

interface Item {
  ...
}

interface GetItemListResponse {
  list: Item[];
}

export const getItemList = async ({
  page = 1,
  pageSize = 10,
  orderBy = "recent",
  keyword = "",
}: GetItemListArg): Promise<GetItemListResponse> => {
  try { ... }
};

activeBtn: boolean;
};

export default function Button({ onClick, children, activeBtn }: ButtonProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
React에서 제공하는 PropsWithChildren 유틸리티 타입을 활용할 수도 있습니다!
그러면 children을 따로 정의하지 않아도 됩니다!

type ButtonProps = {
  onClick: (e: MouseEvent, validateOk: boolean) => void;
  activeBtn: boolean;
};

export default function Button({ onClick, children, activeBtn }: PropsWithChildren<ButtonProps>) {...}

import { MouseEvent } from "react";

type ButtonProps = {
onClick: (e: MouseEvent, validateOk: boolean) => void;
Copy link
Collaborator

Choose a reason for hiding this comment

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

p4;
조금 더 명확하게 MouseEvent가 어디서 발생하는지 명시할 수 있습니다!
현재는 button 요소에서 발생하는 MouseEvent이기 때문에 아래와 같이 작성할 수 있어요.

 onClick: (e: MouseEvent<HTMLButtonElement>, validateOk: boolean) => void;

타입스크립트를 학습하고 이왕 사용하는 김에, 요렇게 구체적인 타입 명시를 하는 습관을 들이면 더 좋을 것 같아요!
지금의 코드에서는 당장 큰 필요가 없지만, 이렇게 buttonElement를 명시한 경우, button의 특화된 속성들이나 메서드에
자동 완성 기능 등을 지원받을 수 있고, 안전하게 button 요소에 접근할 수 있습니다!

value={value}
onChange={() => {
onChange(name, inputRef.current!.value);
}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3;
이 부분에서 ref로 접근하는 이유가 있을까요?
특별한 이유가 없는 한, input의 onChange 이벤트가 발생했을 때, 이벤트가 발생한 요소(event.target)에 접근하여 value를 가져오는 것이 더 자연스러워 보입니다!

  const handleChangeInput = (e: ChangeEvent<HTMLInputElement>) => {
    onChange(name, e.target.value);
  };

 <input onChange={handleChangeInput} />

이렇게 한 경우, inputRef.current! 와 같이 null에 대한 처리를 부자연스럽게 하지 않아도 되고, 명확성도 높아질 것 같아요!
e.target은 항상 이벤트가 발생한 요소를 가리키므로 null이 될 가능성이 없습니다.

return { state, loadItemList, changeQuery };
};

export default useItems;
Copy link
Collaborator

Choose a reason for hiding this comment

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

요 훅에서 조금 힘들더라도, 아래와 같은 부분들을 개선하여 타입 및 인터페이스 정의하는 걸 연습해보면 좋을 것 같아요!

  1. Data 인터페이스에 any를 사용하지 않고 명확하게 지정
  2. reducer의 액션에 대한 인터페이스를 구체적으로 지정
  3. 타입 및 변수를 더 구체적으로 작성 -> 예를 들어 INITIAL_STATE라는 변수 및 타입도 INITIAL_ITEM_FETCHING_STATE 등의 구체적인 이름으로 직관성을 높이면 좋을 것 같아요!

<div className="flex w-full max-w-md flex-col gap-4 md:max-w-4xl lg:max-w-none">
<label className="text-xl font-bold">베스트 상품</label>
<div className="flex justify-center gap-x-3">
{items.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.

p3;
요 부분은 items.length > 0 이 아닌 showItems.length > 0 조건이 더 정확한 것 같아요!

const [inputValue, setInputValue] = useState("");
const inputRef = useRef<HTMLInputElement | null>(null);

const onChange = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3;
이 부분도 위에서 언급드린 것처럼 ref 대신 이벤트 객체에 접근하여 value를 가져오는 게 더 자연스러운 것 같아요!

</button>
</div>
<div className="flex flex-wrap justify-between gap-x-3 gap-y-5">
{items.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.

p3;
여기도 조건 showItems.length > 0 으로 수정하는게 더 정확할 것 같습니다!


const bestItems = useRef<Item[]>([]);
const pageSize = usePageSize();
const showItemNum = useMemo(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

p3;

  1. 변수명이 개선되면 좋을 것 같아요! 현재 showItemNum 이라는 변수명은 실제로 명령형 함수 같은 느낌이에요!
    visibleItemsCount 등의 변수명이 어떨까 싶어요!

  2. 삼항연산자가 중첩되어 있어서 가독성이 떨어지는 것 같아요! 이런 경우 별도의 함수로 구분하여, getVisibleItemsNumber(pageSize) 등으로 값만 가져오거나, switch 문을 사용하면 좋을 것 같아요.

  3. pageSize 별로 관리되는 itemNum의 값은 상수로 관리해도 괜찮을 것 같아요!

ex)

const VISIBLE_ITEMS_COUNT = {
  small: { best: 1, selling: 4 },
  medium: { best: 2 selling: 6 },
  large: { best: 14 selling: 10 }
}

const visibleItemsCount = useMemo(() => {
  return VISIBLE_ITEMS_COUNT[pageSize];
}, [pageSize]);

@Taero-Kim
Copy link
Collaborator

안녕하세요, 세환님!
우선, 프론트엔드 개발의 거의 대부분이 React에 의존하고 있는데, 파트 하나에 소화하실 수 없는 게 당연히 자연스러운 현상입니다.

그런 의미에서 아예 처음부터 만들며 이것저것 시도하고 계신 점, 잘하시고 있으신 것 같아요.
일단 현재 학습하는 타입스크립트도 중요하지만, 리액트가 부족하다고 느껴지신다면 일단 리액트에 더 비중을 실어 학습하시는 게 맞다고 생각합니다.

React 에서도 여러 가지 api들이 있지만, 일단 React의 기본 콘셉트를 다시 차근차근 파악하시고,
기본적인 Hook( useState, useEffect 등)에 대한 개념을 보시면서, 컴포넌트 간 상태 관리의 흐름에 익숙해지시는 게 중요할 것 같습니다.
�이러한 상태 관리의 흐름이 익숙해지시면 Context api나 useReducer 등의 개념을 학습하시고 하나하나 기존 코드를 적용(변경)을 해보면서
그 차이점과 장단점을 익히시는게 좋을 것 같아요!

개인 블로그 등 다양한 학습 소스들이 있지만, 정보들이 너무 파편화되어 있고 종종 잘못된 정보도 많아 혼란을 겪으실 수 있을 것 같아요!
그래서 오히려 다른 것보다 공식 문서를 차근차근 정독하시는 걸 추천드립니다.
React 공식 문서를 한글로 번역한 사이트 링크를 남겨 드릴게요.
https://react-ko.dev/learn

인내하고 천천히 읽다 보면, React에 대한 전반적인 이해도가 높아질 거라 생각합니다.

지금 단계에서는, 이런 패턴이 좋다, 이런 구조가 좋다 여러 가지 정보들에 휘둘리지 마시고,
React 본질에 집중하시어 주도적으로 판단하시어 코드를 작성하시는 걸 추천드리고,
바로 코드에 손을 대기보다는 어떤 로직이나 컴포넌트를 설계할 때 수도코드로, 설계의 흐름을 정리한 뒤에 코드 작성을 하시는 것도 추천드립니다!

아무튼 지금 열심히 하고 계시고, 잘 하고 계시니 큰 걱정 하지 마시고
주변에 휘둘리지 않고 세환 님의 페이스를 잘 유지하시길 바라요!

@Taero-Kim
Copy link
Collaborator

�전체적으로 코드를 보며, 타입스크립트 부분을 위주로
개선하면 좋을 점들을 리뷰로 남겼습니다.

리뷰의 모든 내용을 반영하실 필요는 없고,
느낌만 이해하고 넘어가셔도 좋습니다!

타입스크립트는 특히 계속 코드를 작성하시다 보면, 자연스럽게 익숙해지실거니 너무 걱정 마세요!

+) 리뷰를 남긴 코멘트 위의 p(1~5); 마크의 의미는 아래와 같습니다! 참고하면 좋을 것 같아요!

p1; 꼭 반영해주세요 (Request changes)
리뷰어는 PR의 내용이 서비스에 중대한 오류를 발생할 수 있는 가능성을 잠재하고 있는 등 중대한 코드 수정이 반드시 필요하다고 판단되는 경우, P1 태그를 통해 리뷰 요청자에게 수정을 요청합니다. 리뷰 요청자는 p1 태그에 대해 리뷰어의 요청을 반영하거나, 반영할 수 없는 합리적인 의견을 통해 리뷰어를 설득할 수 있어야 합니다.

p2; 적극적으로 고려해주세요 (Request changes)
작성자는 P2에 대해 수용하거나 만약 수용할 수 없는 상황이라면 적합한 의견을 들어 토론할 것을 권장합니다.

p3; 웬만하면 반영해 주세요 (Comment)
작성자는 P3에 대해 수용하거나 만약 수용할 수 없는 상황이라면 반영할 수 없는 이유를 들어 설명하거나 다음에 반영할 계획을 명시적으로(JIRA 티켓 등으로) 표현할 것을 권장합니다. Request changes 가 아닌 Comment 와 함께 사용됩니다.

p4; 반영해도 좋고 넘어가도 좋습니다 (Approve)
작성자는 P4에 대해서는 아무런 의견을 달지 않고 무시해도 괜찮습니다. 해당 의견을 반영하는 게 좋을지 고민해 보는 정도면 충분합니다.

p5; 그냥 사소한 의견입니다 (Approve)
작성자는 P5에 대해 아무런 의견을 달지 않고 무시해도 괜찮습니다.

@Taero-Kim Taero-Kim merged commit 16d7f6b into codeit-bootcamp-frontend:React-김세환 Aug 5, 2024
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