YeongseoYoon commented 1 year ago

안녕하세요 아이쓰님!!! 윤영서입니다. 어느덧 저희 조 세번째 코드리뷰네요. 앞서 TA이신 태영님께서 리뷰를 해주셨지만, 제가 봤음에도 기억력이 그렇게 좋지 않은터라, 중복된 리뷰가 있어도 그러려니하고 너그럽게 생각해주세요 :) 아! 그리고 제가 글솜씨가 없어서, 이해가 되지 않는 부분이 있을 수 있는데, 그럴 땐 꼭 어떤 의미인지 다시 물어봐주세요!! 다시 답변 드리겠습니다 ㅎㅎ 그럼 시작하겠습니다!

1. Not Found 페이지!

export default function App() {
  return (
    <Router>
      <Switch>
        <Route path="/character/:id">
          <Detail />
        </Route>
        <Route path="/notfound">
          <NotFound />
        </Route>
        <Route path="/">
          <Home />
        </Route>
      </Switch>
    </Router>
  );

react router dom 5버전을 사용하셔서, 저렇게 not found를 path를 통해서 주고 계시네요! react router dom 6버전으로 올려서 사용하시면, errorElement를 통해 에러 캐치가 가능해집니다.

const router = createBrowserRouter([
  {
    path: '/',
    element: <App />,
    errorElement: <ErrorBoundary />,
    children: [
      {
        path: '',
        element: <Home />,
      },
      {
        path: 'detail/:issueNumber',
        element: <Detail />,
      },
    ],
  },
]);

https://reactrouter.com/en/main/route/error-element 각 children path에 대해서도 errorElement 지정이 가능해요! 자세한 부분은 react router dom 문서를 읽어보시면 좋을 것 같습니다.

2. 감춰진 진짜

코드는 자기 자신이 보는 것도 중요하지만, 다른 개발자들과 소통하기 위해, 명확하고, 깨끗하게 작성되어야 합니다. 클린 코드, 클린 아키텍처가 각광받는 것도 그런 이유가 아닐까요?

아이쓰님의 코드를 한번 봐볼게요!

//Character.js

import React, { useRef } from "react";
import { useEffect, useState } from "react";
import {
  Link,
  useHistory,
  useParams,
} from "react-router-dom/cjs/react-router-dom.min";
import styled, { keyframes } from "styled-components";
import Comic from "../Components/Comic";
import Loading from "../Components/Loading";

const TITLE_FIX_Y = 20;

const Back = styled.div`
  width: 20px;
  height: 20px;
  border-radius: 50%;
  background-color: #ff3939;
  position: fixed;
  top: 20px;
  left: 20px;
  cursor: pointer;
  z-index: 1;
`;

const BackImg = styled.img.attrs((props) => ({
  src: props.src,
}))`
  object-fit: cover;

  filter: blur(16px);

  width: 100%;
  height: 100%;
  transform: scale(1.05);
  position: fixed;
  top: 0;
  left: 0;

  z-index: -1;
`;

const Container = styled.div`
  display: flex;
  justify-content: flex-start;
  align-items: center;
  flex-direction: column;
  position: relative;
  padding-bottom: 60px;
`;

const XROTATE = 5;
const imgAnimation = keyframes`
    0%{
        transform: rotateX(${XROTATE}deg) rotateY(0deg);
    }
    10%{
      transform: rotateX(${XROTATE}deg) rotateY(30deg);
    }
    50%{
        transform: rotateX(${XROTATE}deg) rotateY(300deg);
    }
    100%{
        transform: rotateX(${XROTATE}deg) rotateY(360deg);
    }
`;

const ImgContainer = styled.div`
  width: 300px;
  height: 520px;
  margin-top: 50px;
  overflow: hidden;
  box-sizing: border-box;
  position: relative;
`;
const ImgBox = styled.div`
  position: absolute;
  top: 0;
  left: 0;
  width: 300px;
  height: 460px;
  border: 2px solid white;
  box-sizing: border-box;
  border-radius: 20px;
  animation: ${imgAnimation} 1.5s linear;
  backface-visibility: ${(props) => (props.backface ? "visible" : "hidden")};
  z-index: ${(props) => (props.$zIndex ? props.$zIndex : 0)};
  transform: rotateX(${XROTATE}deg);
  margin-top: 30px;
`;
const ImgTemplate = styled.img`
  width: 100%;
  height: 100%;
  box-sizing: border-box;
  border-radius: 20px;
  object-fit: cover;
`;
const Img = styled(ImgTemplate).attrs((props) => ({
  src: props.$imgurl,
}))`
  background-color: #ededed2e;
`;
const ImgBack = styled(ImgTemplate).attrs((props) => ({
  src: props.$imgurl,
}))`
  filter: grayscale(95%);
`;

const Title = styled.h2`
  margin-top: 48px;
  margin-bottom: ${TITLE_FIX_Y}px;
  color: white;
  font-size: 46px;
  font-style: italic;
  text-shadow: 4px 2px 2px black;
  position: sticky;
  top: 20px;
  transition: all 0.3s ease-in-out;
  padding: 16px 0;
  text-align: center;
  border-radius: 20px;
  &.bgDark {
    background-color: #202020db;
    padding: 16px 28px;
    transform: scale(1.1);
  }
`;

const Desc = styled.div`
  margin-top: 12px;
  margin-bottom: 24px;
  width: 60%;
  color: white;
  font-size: 24px;
  font-style: italic;
  text-shadow: 1px 1px 6px black;
`;

const More = styled.a.attrs((props) => ({
  href: props.$link,
  target: "_blank",
}))`
  width: 260px;
  height: 70px;
  margin-top: 40px;
  background-color: #ffffff91;
  display: flex;
  justify-content: center;
  align-items: center;
  border-radius: 36px;
  font-size: 24px;
  font-weight: bold;
  cursor: pointer;
  color: black;
  text-decoration: none;
  margin-bottom: 60px;
  box-shadow: 12px 7px 20px 0px #00000099;
`;

const ComicList = styled.div`
  width: 60%;
  display: flex;
  flex-direction: column;
  justify-content: flex-start;
  align-items: center;
`;

//로직 코드
const Detail = () => {
}

위 부터 여기까지 내려오시면서, 되게 길다! 라는 느낌이 드셨을 수 있을 거 같아요.

styled component를 한 파일에 함께 작성해두셨네요! 그렇지만, 파일 앞부분에 css가, 그리고 아래에 진짜 코드가 놓여있어 진짜는 여깃지롱! 의 느낌이 들곤 합니다. 정답은 아니지만, 가독성을 위해서 저는 이렇게 작성할 것 같아요.

import React, { useRef } from "react";
import { useEffect, useState } from "react";
import {
  Link,
  useHistory,
  useParams,
} from "react-router-dom/cjs/react-router-dom.min";
import styled, { keyframes } from "styled-components";
import Comic from "../Components/Comic";
import Loading from "../Components/Loading";

const TITLE_FIX_Y = 20;

//로직
const Detail = () => {
}

//디자인 어쩌고
const Back = styled.div``;

이렇게 하면, 눈에 딱! 로직이 보이고, 비교적 덜 중요한 스타일 관련 파트를 뒤쪽에 배치해 더욱 가독성 좋은 코드를 보실 수 있을 거에요.

const XROTATE = 5; 그리고 중간 중간에 이런 매직넘버를 상수화한 코드가 숨겨져 있는데, 상수와 같은 코드는 제일 위에 작성해주시는게 좋습니다. 요게 어딨지? 라며 매번 ctrl+f를 통해 찾아야 하는 불상사가 일어날수도..?

3. 너의 이름은

  <Desc>{data.description}</Desc>

아이쓰님이 코드에서 Desc라고 작성을 해주셨어요! 많은 개발자들은 desc를 보고 이렇게 생각할 겁니다. 17e3ce55d514ff843

Tkdodo 형님은 이렇게 말씀하십니다. MZ들아 줄이지마라! 죄송합니다 사실 이렇게 말씀하시진 않았고, 암튼 No abbreviations라곤 하셨네요. 변수명을 줄여쓰지 말라는 의미입니다. https://tkdodo.eu/blog/on-naming-things#no-abbreviations Desc대신 Description으로 작명해주는 것이, 더 명확하겠네요!

4. 플로우 오버플로우 그리고... reflow

아이쓰님이 getBoundingClientRect() 를 사용하셨더라고요! 아마 스크롤 시에 위 타이틀쪽을 고정시키려는 의도가 아니셨을까 합니다.

  useEffect(() => {
    const onScroll = () => {
      if (!titleRef.current) return;
      setIsScrolled(
        titleRef.current.getBoundingClientRect().top <= TITLE_FIX_Y
      );
    };

그렇지만, 이 getBoundingClientRect()는 reflow를 발생시킵니다.

Reflow란?

일반적인 브라우저 렌더링 과정은, DOM -> CSSOM ->이 둘을 합쳐 Render Tree -> Layout -> Paint 과정을 거치게 되는데 여기서 Layout 단계를 다시 하게 되는 것을 의미합니다.

저도 자세히 브라우저 렌더링을 공부한 것은 아니라, 대충 이렇게 이해하고 있습니다만 자세한 브라우저 렌더링 과정은, 브라우저는 어떻게 동작하는가? 이 글을 참고하시면 좋을 것 같습니다 ㅎㅎ

아무튼 레이아웃을 다시 계산해 그리는 것은 생각보다 가벼운 연산이 아닙니다. 하위 언급된 toast ui 기술 블로그에서도, 영향받는 모든 노드(자기 자신, 자식 노드, 부모 노드, 조상 노드)의 값을 재계산하여 렌더 트리를 업데이트 한다고 언급하고 있습니다. 영향받는 모든 노드의 값을 재계산하려고 하면 당연히 연산이 가볍지만은 않겠죠? ㅎㅎ

아이쓰님의 정확한 의도는 제가 코드를 실행해보지 않아 모르겠으나, getBoundingClientRect()의 reflow에 대해 알아두시면 좋을 것 같아 한 번 소개드렸습니다 ㅎㅎgetBoundingClientRect()를 대신할 만한 코드를 작성하면 더 좋겠네요. 그것이 아니라면, 최적화 할 만한 방법을 생각해 보는 것이 좋을지도? 헤헤 https://ui.toast.com/fe-guide/ko_ANTI-PATTERN#%EB%B6%88%ED%95%84%EC%9A%94%ED%95%9C-%EB%A0%88%EC%9D%B4%EC%95%84%EC%9B%83%EC%9D%84-%EB%B0%9C%EC%83%9D%EC%8B%9C%ED%82%A4%EC%A7%80-%EC%95%8A%EB%8A%94%EB%8B%A4

5. Guitar 🎸(대충 기타)

      {datas === null ? (
        <Loading />
      ) : (

이런건

      {datas ? (
        <Loading />
      ) : (

이렇게만 해줘도 충분해요!

 let urls = data?.urls.filter((url) => url.type === "comiclink");

let를 쓰셨네요! let은 개발자도 모르는 사이에 값이 바뀔 위험성이 있어서, const를 쓰라고 권장하긴 합니다! 저라면 그냥 url을 불러낼때, map내부에서, url에 comiclink가 있으면 부르는 코드로 작성할 것 같아요!

쓰다보니, 에너지가 많이 나가서 하하,,, 얼리리턴 영떠 해버리겠습니다...

늦게 작성해서 죄송해요! 여행에 왔는데 어쩌다보니 늦게 도착해서,,,다음날 작성해버렸네요...헤헤

사실 작성해드리고 싶은 말들이 더 있긴했는데, 내용이 miffy(chanbin)님과 또자님께 해드린 리뷰와 비슷한 점들이 있어서, 고 부분은 다른 분들 리뷰 참고하시면 좋겠습니다 ㅎㅎ

리뷰 맡겨주셔서 넘 감사해요! 제가 하는 말들이 다 정답은 아니고 저도 공부한지 얼마 되지 않은 입장이라, 그냥 이런게 있구나 정도로만 생각해주세요!

BBAKJunhyeong commented 1 year ago

LGTM LGTM LGTM!!!!!!!!!!!

ICE0208 commented 1 year ago

@YeongseoYoon 안녕하세요 영서님~! 소중한 코드 리뷰 감사드립니다! 천천히 읽어보면서 든 생각을 영서님 스타일(?)에 맞게 코멘트를 작성해보았습니다 ㅎㅎ

1. 스포 감사합니다(?)

아직 ReactJS 마스터클래스 강의를 듣고 있어서 5버전으로 진행하였는데 6버전에는 저런 것도 있군요! 6버전으로 다시 공부할 생각에 벌써 신나네요~

2. 오 이게 되네?

위로 올려도 정상작동 하는군요…! 역시 전 자바스크립트 기초부터 다시 공부해야할 것 같습니다ㅜㅜ 안녕히계세요(?) 😂

3. 이 작품 좋아하는 거 어떻게 아셨찌

ㅋㅋㅋㅋ 앞으로는 풀네임으로 작성하겠습니다ㅏㅏㅏ 좋은 지적 감사해요!!

4. 공부.. 해야겠다..

이 부분은 시간을 들여서 공부를 해봐야겠네요! 주신 링크 활용해서 공부해보겠습니다~

5. 기타.. 배워보고 싶네요

datas가 없을 때 Loading을 띄워줘야하는데

{!datas ? ( // <- 느낌표 추가
  <Loading />
) : (

이거를 의도하고 말씀주신거 맞나요??

오늘 벌써 let으로 두 번 지적(?) 당했네요! 앞으로는 const/let 을 구분지어서 써보겠습니다!

6. 여담

죄송하다니요! 저의 어질어질한 코드를 리뷰해 주신 것만으로도 감사합니다. 많은 도움이 되었습니다~ 역시 갓영서님!!! 🥳