첫 번째 리뷰 요청 입니다. #12

MuseopKim · 2021-02-08T16:09:49Z

@choichoigang 호이 안녕하세요.

첫 번째 리뷰 요청을 드립니다.

구현 내용

Main UI
신청곡 목록
신청곡 상세
신청곡 등록 / 수정 폼
신청곡 검색 UI
(+) 배포 전이라 아직 mock 데이터를 사용하고 있습니다.
이제 다시 백엔드 개발 예정인데, 아마도 다음 리뷰 요청부터 서버에서 데이터를 받아올 듯 합니다.

API 문서

아직 구현된 기능이 많지 않아서 큰 내용은 없지만 요청과 응답 JSON을 보시면 더 이해하시기 좋을거 같아 첨부 합니다. 문서는 앞으로도 매 리뷰 요청마다 따로 첨부 해드리겠습니다.

링크

리액트로 처음 하는 프로젝트이다보니 많이 어려웠습니다.

핵심 로직은 평범한 CRUD에서 크게 벗어나지 않는 듯 한데, 모달 형식으로 같은 UI 안에서 CRUD를 모두 구현 하려다 보니 상태관리와 코드가 점점 복잡해지고 있음을 느낄 때가 많았습니다. 아직 익숙하지 않아서인지 커밋 또한 의미 있는 단위로 관리하지 못한 듯 하네요.

코드를 보시면서 상태관리나 방향성에 대한 조언을 중간중간 해주신다면 큰 도움이 될거 같습니다. 🙇🏻‍♂️

아낌 없는 조언을 부탁 드립니다.

어렵게 말씀을 드렸는데 흔쾌히 리뷰를 수락 해주셔서 감사합니다!

- Redux - react-redux - react-router-dom - styled-components

GlobalStyle - styled-reset 적용 - rem 사용의 편의성을 위해 기본 폰트 사이즈를 62.5%로 설정

Header - HeaderTitle - Navigation - UserProfile

react-icons

Header, HeaderTitle, Navigation, UserProfile

이질감을 줄이기 위해 shadow 색상을 적정 값으로 수정

index에는 App 컴포넌트만 남겨두기 위해 수정

- "/" : 신청곡 목록 - "/ranking" : 신청곡 순위 - "/contents" : 관련 유튜브 컨텐츠 - "/musicsheets" : 악보집 목록

import 경로 중복을 제거하기 위해 추가

- 반응형을 고려해서 max-width 값으로 변경

Main 레이아웃 정의

import 경로 중복 제거를 위해 추가

중복 코드를 줄이기 위하여 MainTemplateBlock에 있던 width, height 값들을 css body 속성으로 넘기고, Template에는 max-width 값 추가

- 개별 신청곡 및 사연의 상태를 관리하는 Letter, LetterList 컴포넌트 추가 - mock 데이터를 사용하여 테스트

전체 레이아웃을 위해 푸터 컴포넌트를 미리 추가. 임시 텍스트 입력

- 앨범 이미지와 겹치는 현상이 있어서 fixed 헤더에 z-index 값 부여 - 신청곡 리스트 margin-top 값 수정

- margin으로 중앙정렬 하는 대신 상위 요소에서 flex 정렬을 사용 하도록 변경 - 각 메인 컴포넌트 코드의 중복을 없애기 위해 MainTemplate 하위에 Block 컴포넌트를 하나 더 추가하여 메인 레이아웃 고정

api/Letters - 현재는 Mock 데이터를 받아오지만 추후 axios를 통한 REST 통신을 관리

- 컴포넌트가 아니므로 이름 전체를 소문자로 변경 - Promise syntax error 수정

- 현재는 useState로 로 mock데이터를 받아오도록 설정 - 추후 store 구현 이후 redux로 상태 관리 예정

…-request-frontend into feature/issue-2

전체 목록 조회 UI

페이지별 컴포넌트 생성을 위해 미리 이동

컴포넌트 이름의 변경

- ModalTemplate : 검색 화면에서의 재사용을 위해 기본 모달 화면 구성 템플릿 추가 - LetterDetails : Letter 상세 페이지 컴포넌트

- 비동기 처리 과정에서 커스텀 미들웨어 사용

- useModal : 모달 조건 설정 공통화 및 활성화 된 Letter 상태 관리를 위해 Custom Hook 사용

레이아웃 재정렬 및 몇몇 값들 수정

custom hook -> 스토어 관리로 폼 상태관리 변경

letter 데이터를 직접 받아와서 바인딩 하도록 수정

선택한 신청곡에 대한 결과가 폼에 바인딩 되도록 구현

READ -> EDIT 전환 중 UI 흔들리는 현상 수정

현재는 mock 데이터를 사용하도록 구현

API 연동 이후 본 로직 수정 예정

수정 이후 Letter 리스트 refetch 과정에서 영향을 받지 않도록 별도로 분리

일정 단위 이상의 컴포넌트 별도의 패키지로 분리

분리 할 필요가 없는 컴포넌트 병합

글자수 제한, 레이아웃 재정리

Letter 조회 / 수정 및 mock 데이터 연동

Letter 상세조회 / 수정 Modal 및 mock 데이터 연동

eslintcache 오류로 인해 반영 안되던 수정 파일 다시 생성

READ <-> EDIT 전환 과정에서 레이아웃 흔들리던 현상 수정

eslintcache 오류 및 Song 폼 레이아웃 수정 반영

choichoigang

이번 리뷰에서 개인적으로 개선하면 좋을거 같은 부분을 몇개 말하고 마무리 하도록 하겠습니다 !

스타일과 관련된 코드를 테마화하고 그 값들을 공동적으로 사용해 보는건 어떨까요 ? margin으로 간격을 주는 사소한 부분도 공동적으로 관리하면 일관된 레이아웃을 가져갈 수 있을거라고 생각합니다.
모달이 페이지에서 엄청 중요한 역할을 하는거 같은데 스타일을 좀 바꿔봐도 좋을거 같아요. 예들들면 웹에서는 지금보다 더 큰 레이아웃의 모달을 보여주고 모바일에 대응될 땐 화면을 꽉 채워서 제공하는 모달의 형태도 좋을거 같다는 생각이 듭니다 !
전반적으로 컴포넌트가 간결해서 보기 좋았습니다 ! 좀 더 개선하자면 컴포넌트를 한단계 더 묶을수 있을 거 같다는 생각이 드는 부분이 있었는데 그 부분은 리뷰를 보시고 한번 생각해봐도 좋을거 같아요 !
custom hook로 다양한 시도를 해보신거 엄청 좋다고 생각합니다. 다음번에는 복잡한 라이프사이클을 순회하는 custom hook 만들기에 도전해 보셔도 좋을거 같아요 ! ( 복잡하다고 했지만 사실 그렇게 복잡하진 않습니다 ㅋㅋ )

choichoigang · 2021-02-09T12:03:48Z

src/api/letters.js

+  const { title, artist, imageUrl } = payload;
+  const songStory = payload.songStory;
+
+  const foundLetter = letters.find(letter => letter.id === id);


배열에서 원하는 조건까지만 찾는 find method를 사용한 건 좋아보입니다 !

choichoigang · 2021-02-09T12:09:10Z

src/components/Footer/Footer.jsx

+  margin-top: 6rem;
+  background-color: #f5f5f7;
+  font-weight: 500;
+  box-shadow: 0px -10px 35px 5px #f1f1f1;


style components를 사용하시니까

box-shadow: 0px -10px 35px 5px #f1f1f1;

부분의 색상 같은 경우는 styled-components ThemeProvider를 사용해 보는건 어떨까요.
모든 컴포넌트가 전달받은 색상을 동일하게 사용하면 다크모드와 같은 기능을 구현할때도 손쉽게 스타일 변동이 가능합니다.
너무 많이 진행된 프로젝트는 스타일 시스템을 구축하기 쉽지 않으니까 좀 작은 단위일 때 시도해 보는것도 좋을거 같아요 😉

컴포넌트를 재사용 하듯이 스타일도 여러 곳에 재사용 하는 방법이 있었네요. ThemeProvider에 대해 잘 몰랐는데 이번 기회에 한번 학습 해보고 해볼 수 있다면 적용까지 해보겠습니다! 조언 해주셔서 감사합니다 👍

choichoigang · 2021-02-09T12:12:04Z

src/components/Header/Navigation.jsx

+    align-items: center;
+    margin: 0px 30px;
+    padding-top: 10px;
+    font-size: 1.2rem;


폰트와 같이 반응형에 영향을 받아야 하는 기준과 그렇지 않은 요소를 기준으로 px과 rem을 사용하시는 건가요 ?
그런 의도라면 좋아보입니다.

처음에는 말씀 하신 것처럼 px과 rem을 구분해서 사용 하다가 상대 값인 rem으로 단위를 통일하고 적용 했습니다. 혹시 이렇게 할 경우 주의해야 될 고려사항 같은게 있을까요?

choichoigang · 2021-02-09T12:14:13Z

src/components/Letter/Letter.jsx

+          <img src={imageUrl} alt="ALBUM IMAGE" className="album-image" />
+          <div className="song-about">
+            <span className="song-about__title">{title}</span>
+            <span className="song-about__artist">{artist}</span>


className 컨벤션 중 "-"와 "_"는 어떤 기준이 있을까요 ? 궁금합니다 ㅎ

컴포넌트 이외에 className을 직접 사용할 때는 대부분 CSS 네이밍 컨벤션으로 알려진 BEM 을 적용 했습니다ㅎㅎ
-는 두 단어를 연결해야 할 때, __는 특정 블럭을 구성하는 하위 요소를 구분할 때 사용 했던 듯 하네요.
CSS BEM 을 참고 했습니다 :)

choichoigang · 2021-02-09T12:17:06Z

src/components/Letter/LetterList.jsx

+  return (
+    <LetterListBlock>
+      {letters.map(letter => (
+        <LetterContainer


보통 Container는 좀 더 큰 단위에서의 의미에서 사용되곤 하는거 같아요.
여러 자식 컴포넌트를 가지고 많은 이벤트 로직을 가지고 있는 센터의 느낌으로 네이밍을 하곤 하는데
LetterList면 LetterItem과 같이 단일하게 표현해 주는것도 좋을거 같아요 !

Container를 언제 사용해야 할 지 명확한 기준이 없었는데, 리뷰 해주신 내용을 보고 고민 되던 것들이 많이 덜어지는 느낌이에요.
다음 수정 때 반영 하겠습니다 🙂

choichoigang · 2021-02-09T12:57:36Z

src/containers/Letter/LetterListContainer.jsx

+  if (error) {
+    return <div>ERROR!</div>;
+  }


나중에 에러처리를 할 때 400번대와 500번대의 중요 status는 조건을 고려해서 컴포넌트로 사용하면 좋더라구요 !

에러 처리는 깊게 생각해보지 못했는데 기억 하고 있다가 적용해보겠습니다! 👍

choichoigang · 2021-02-09T13:00:07Z

src/containers/LetterEditor/LetterEditContainer.jsx

+  const onChange = event => {
+    const { name, value } = event.target;
+    dispatch(updateForm(name, value));
+  };


객체 분해를 사용한 건 좋아보입니다 ! 저는 요즘 ( { target : { value , name } } ) 이런식으로 인자에서 바로 분해해서 사용하기도 하는데 함수를 한줄로 표현하기 편해져서 좋더라구요 ㅎ 각각 장점이 있는거 같습니다.

아직 구조 분해 할당이 완벽히 익숙하지는 않지만 말씀 해주신 경우처럼 직관성을 해치지 않는 선에서는 바로 받아와 사용하는게 더 간결해 보이네요 🙂

choichoigang · 2021-02-09T13:04:16Z

src/containers/LetterModal/LetterModalContainer.jsx

+    document.body.style.overflow = "hidden";
+    document.body.scroll = "no";


모달이 열리면 background가 스크롤되는 현상을 막기 위한 방법인가요 ?

네 맞습니다ㅎㅎ background에서 스크롤 되는 현상을 막기 위함인데 혹시 더 효율적인 방법이 있을까요?

choichoigang · 2021-02-09T13:04:38Z

src/containers/LetterModal/LetterModalHiddenButtonContainer.jsx

+  const changeToEdit = () => {
+    dispatch(changeModalType(LETTER_MODAL.EDIT));
+  };


여기는 한줄로 표현 가능할 듯 합니다 ㅎ

다른 로직을 추가 했다가 지우는 과정에서 놓친거 같네요. 다음번 수정 할 때 반영 하겠습니다 👍 알려주셔서 고맙습니다!

choichoigang · 2021-02-09T13:07:15Z

src/modules/letter.js

+const UPDATE_LETTER_ERROR = "letter/UPDATE_LETTER_ERROR";
+
+export const getLetterById = id => async dispatch => {
+  dispatch({ type: GET_LETTER });


해당 middleware의 dispatch({ type: GET_LETTER });는 스토어의 상태를 초기화 해주는 건가요 ?

fetch 요청에 대한 응답이나 에러가 반환 되기 전까지 스토어 상태를 로딩으로 유지 해주는거라 생각 해주시면 될거 같습니다 :)

MuseopKim added 30 commits January 27, 2021 09:25

[#1] Chore: 필요 라이브러리 추가

b039a67

- Redux - react-redux - react-router-dom - styled-components

[#2] Chore: styled-reset 추가

ce0dfa5

[#2] Feat: Reset, GlobalStyle 설정 및 적용

29bea5d

GlobalStyle - styled-reset 적용 - rem 사용의 편의성을 위해 기본 폰트 사이즈를 62.5%로 설정

[#2] Feat: Header 컴포넌트 추가

75fe662

Header - HeaderTitle - Navigation - UserProfile

[#2] Chore: react-icons 모듈 추가

b8b8d60

react-icons

[#2] Feat: Header 컴포넌트 기본 스타일링 적용

6a4bce2

Header, HeaderTitle, Navigation, UserProfile

[#2] Fix: Header 컴포넌트 box-shadow 수정

79e5acd

이질감을 줄이기 위해 shadow 색상을 적정 값으로 수정

[#2] Fix: index.js에 있던 컴포넌트를 모두 App으로 이동

2b251d3

index에는 App 컴포넌트만 남겨두기 위해 수정

[#2] Feat: 라우터 추가

6f29986

- "/" : 신청곡 목록 - "/ranking" : 신청곡 순위 - "/contents" : 관련 유튜브 컨텐츠 - "/musicsheets" : 악보집 목록

[#2] Fix : SongRequest 컴포넌트 디렉토리에 index.js 추가

55f373d

import 경로 중복을 제거하기 위해 추가

[#2] Fix: Header 컴포넌트 수정

6d29f08

- 반응형을 고려해서 max-width 값으로 변경

[#2] Feat: MainTemplate 추가

a42983d

Main 레이아웃 정의

[#2] Feat: Header 컴포넌트 index.js 추가

6edbd97

import 경로 중복 제거를 위해 추가

[#2] Fix: MainTemplate 레이아웃 값 수정

b01877f

중복 코드를 줄이기 위하여 MainTemplateBlock에 있던 width, height 값들을 css body 속성으로 넘기고, Template에는 max-width 값 추가

[#2] Feat: Letter, LetterList 컴포넌트 추가

0ea96ed

- 개별 신청곡 및 사연의 상태를 관리하는 Letter, LetterList 컴포넌트 추가 - mock 데이터를 사용하여 테스트

[#2] Feat: Footer 컴포넌트 추가

e060ece

전체 레이아웃을 위해 푸터 컴포넌트를 미리 추가. 임시 텍스트 입력

[#2] Feat: LetterList 스타일링 적용

8ac731c

[#2] Fix: 신청곡 목록 레이아웃 수정

954cb0c

- 앨범 이미지와 겹치는 현상이 있어서 fixed 헤더에 z-index 값 부여 - 신청곡 리스트 margin-top 값 수정

[#2] Feat: API 요청 분리

ed7ed09

api/Letters - 현재는 Mock 데이터를 받아오지만 추후 axios를 통한 REST 통신을 관리

[#2] Fix: api/letters 내용 수정 및 이름 수정

709c409

- 컴포넌트가 아니므로 이름 전체를 소문자로 변경 - Promise syntax error 수정

[#2] Feat: LetterListContainer 추가

5d15956

- 현재는 useState로 로 mock데이터를 받아오도록 설정 - 추후 store 구현 이후 redux로 상태 관리 예정

[#2] Feat: LetterListContainer 추가

9d0b61b

- 현재는 useState로 로 mock데이터를 받아오도록 설정 - 추후 store 구현 이후 redux로 상태 관리 예정

Merge branch 'feature/issue-2' of https://github.com/playthesong/song…

d117734

…-request-frontend into feature/issue-2

Merge pull request #3 from playthesong/feature/issue-2

659b30c

전체 목록 조회 UI

[#4] Fix: SongRequest 컴포넌트 pages 패키지로 이동

d5c9a7b

페이지별 컴포넌트 생성을 위해 미리 이동

[#4] Fix: SongRequest -> Letters 변경

f39e066

컴포넌트 이름의 변경

[#4] Feat: Letter 상세조회 기본 컴포넌트 및 ModalTemplate 추가

5fd23e8

- ModalTemplate : 검색 화면에서의 재사용을 위해 기본 모달 화면 구성 템플릿 추가 - LetterDetails : Letter 상세 페이지 컴포넌트

[#4] Feat: Letters 상태관리 리듀서, Redux store 추가

3674557

- 비동기 처리 과정에서 커스텀 미들웨어 사용

[#4] Feat: 신청곡 상세조회 상태 관리 추가

1c468cb

- useModal : 모달 조건 설정 공통화 및 활성화 된 Letter 상태 관리를 위해 Custom Hook 사용

MuseopKim added 25 commits February 6, 2021 17:55

[#8] Fix: 신청곡 검색 모달 레이아웃 정리

8917e70

레이아웃 재정렬 및 몇몇 값들 수정

[#8] Feat: Form 관련 상태를 스토어에서 관리 하도록 letterForm 추가

6cf70bf

[#8] Refactor: 신청곡 수정 폼 상태관리 변경

f244628

custom hook -> 스토어 관리로 폼 상태관리 변경

[#8] Fix: letterForm 모듈 수정

8107df5

letter 데이터를 직접 받아와서 바인딩 하도록 수정

[#8] Feat: 수정 모달 입력 값과 폼 상태값 바인딩 완료

20a51f5

[#8] Feat: 신청곡 선택 결과 form 상태 값에 반영

4f88e39

선택한 신청곡에 대한 결과가 폼에 바인딩 되도록 구현

[#8] Fix: 사연 수정 화면 레이아웃 고정

1bd9600

READ -> EDIT 전환 중 UI 흔들리는 현상 수정

[#8] Fix: Modal 닫힌 뒤 스크롤이 되지 않던 문제 수정

12e5bf6

[#8] Fix: 모달 Open 상태를 최상단 컴포넌트 (LetterModalContainer)에서 관리 하도록 수정

55b5ba1

[#8] Feat: Letter 수정 기능 구현을 위한 스토어 모듈 추가

94b1131

현재는 mock 데이터를 사용하도록 구현

[#8] Feat: mock 데이터를 활용하여 수정 기능이 작동 하도록 구현

f681b04

API 연동 이후 본 로직 수정 예정

[#8] Fix: Letter 모달 컴포넌트 List와 별도로 분리

e067ae8

수정 이후 Letter 리스트 refetch 과정에서 영향을 받지 않도록 별도로 분리

[#8] Refactor: components, containers 패키지 재정리

26d4731

일정 단위 이상의 컴포넌트 별도의 패키지로 분리

[#8] Refactor: LetterModalUser 컴포넌트 일부 병합

3aecafd

분리 할 필요가 없는 컴포넌트 병합

[#8] Fix: 신청곡 리스트 세부 디자인 변경

bb1e1a7

글자수 제한, 레이아웃 재정리

[#8] Fix: 헤더, 메인에 Padding 값 추가

fa113e2

[#8] Fix: 불필요한 Header, Footer index 파일 삭제

0cf979c

[#8] Fix: SongStory Padding 값 조정

f42cfde

[#8] Fix: Footer 색상 변경

318d34d

Merge pull request #9 from playthesong/feature/issue-8

e421573

Letter 조회 / 수정 및 mock 데이터 연동

[#8] Feat: songstory max length 지정

20326a5

Merge pull request #10 from playthesong/feature/issue-8

2c83793

Letter 상세조회 / 수정 Modal 및 mock 데이터 연동

[#8] Fix: api/letter 파일 git 오류 수정

ab51480

eslintcache 오류로 인해 반영 안되던 수정 파일 다시 생성

[#8] Fix: 신청곡 입력 폼 Padding 값 변경

a61c19b

READ <-> EDIT 전환 과정에서 레이아웃 흔들리던 현상 수정

Merge pull request #11 from playthesong/feature/issue-8

610167f

eslintcache 오류 및 Song 폼 레이아웃 수정 반영

MuseopKim added Frontend Review labels Feb 8, 2021

MuseopKim requested a review from choichoigang February 8, 2021 16:09

MuseopKim self-assigned this Feb 8, 2021

choichoigang reviewed Feb 9, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

첫 번째 리뷰 요청 입니다. #12

첫 번째 리뷰 요청 입니다. #12

MuseopKim commented Feb 8, 2021 •

edited

Loading

choichoigang left a comment

choichoigang Feb 9, 2021

choichoigang Feb 9, 2021

MuseopKim Feb 12, 2021

choichoigang Feb 9, 2021

MuseopKim Feb 12, 2021

choichoigang Feb 9, 2021

MuseopKim Feb 12, 2021

choichoigang Feb 9, 2021

MuseopKim Feb 12, 2021

choichoigang Feb 9, 2021

MuseopKim Feb 12, 2021

choichoigang Feb 9, 2021

MuseopKim Feb 12, 2021

choichoigang Feb 9, 2021

MuseopKim Feb 12, 2021

choichoigang Feb 9, 2021

MuseopKim Feb 12, 2021

choichoigang Feb 9, 2021

MuseopKim Feb 12, 2021

		document.body.style.overflow = "hidden";
		document.body.scroll = "no";

첫 번째 리뷰 요청 입니다. #12

Are you sure you want to change the base?

첫 번째 리뷰 요청 입니다. #12

Conversation

MuseopKim commented Feb 8, 2021 • edited Loading

구현 내용

API 문서

choichoigang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuseopKim commented Feb 8, 2021 •

edited

Loading