코드 리뷰1

전체

• 코딩 컨벤션 적용 필요
  • AirBnb 나 Google 쪽을 기반으로 정했으면 함.
  • 에디터에서 해당 설정을 적용했으면 함. 
    • 주로 어떤 에디터를 사용해서 작업하는지요?
    • 프로젝트 룸을 만들고, 누가 설정 가이드도 올렸으면 합니다.
  • tabsize 와 tabexpand 체크 되는 거 맞죠?
  • 메소드 이름 짓는 방식도 통일이 필요해보입니다.
    • onChange vs change
    • 명명규칙
• 디렉토리 구조에 대한 설명 좀 부탁합니다.
  
  • Atomic Design  구조는 어떤지?

• 컴포넌트에서 너무 많은 일을 하려고 하는 듯 보입니다.
  
  • 큰 프로젝트에서는 좋은 방식은 아닌 듯 보입니다.
• ESLINT 항상 확인해주세요.
  • vscode 를 쓰는데, 간단한 버그들 예를 들여 == 대신에 === 를 사용해야하는 케이스들이 걸려나옵니다.

 

package.json

• 왜 axios 가 devDependency 임?
  
  • dependency - 같이 설치되어야 하는 패키지
  • devDependency - 빌드할 때만 필요한 패키지

 

App.js

• 왜 HashRouter 를 쓰고 있는지?
  • BrowserRouter 와 어떤 차이가 있나요?
    • 브라우저 라우터는 프론트와 백로직(ex node)등이 같이 있는 경우 사용한다.
  • HashRouter 특징
    • url에 #!가 포함되어 있다.

 

 

routes/kaban.js

 

• Switch 를 쓰는 의미는 무엇인가요?
• 중첩 라우팅 쓰는 방식이 좀 다른데요. 정확한 방법으로 통일시킬 필요가 있음.
  • MyChannel.js 쪽과 다름.
  • 보통 아래와 같이 쓰고 있음.

Function App() {   return ( <Router>   <Route path='/abc' component = {ABC} </Router> ); }     Function ABC({ match }) {    return (        <>        <Route exact path={match.path} component={ABC_exact} />        <Route path={`${match.path}/:id`} component={ABC_ID} />        </>  ) }

• 라우터에 컴포넌트 매핑하는데 왜 Page 컴포넌트를 만들어서 매핑하지 않고, 여기서 하위 컴포넌트를 여기서 나열하고 있는데 이유가 있을까요?
  • 어차피 Props 넘기는 것은 별반 차이가 없을 텐데...
  • 위 사항관련해서 아마도 routes 에 있는 컴포넌트를 탑 컴포넌트라고 여기고 userID 를 넘기려고?
  • 라우터 안에는 순수하게 이런 정도만 있었으면 함.
    • 기본 컴포넌트
    • 라우팅 경로 -> 컴포넌트
  • userId 넘기는 부분은 mobx 나 react context 를 써서 처리하는 것이 필요함
• 라우터를 디렉토리로 따로 분리해서 관리할 필요가 있을까요?
  • 파일로 만들고 최상위 라이팅 룰만을 기술하고 각각의 페이지 컴포넌트에서 중첩 라우팅으로 하위 라우팅을 처리하는 것은 어떤지?

 

 

components/footer.js

• Button 이라는 컴포넌트가 정의될 필요가 있음(아톰)
  • <button> 태그를 직접 사용하지 말고 React 컴포넌트로 감싸는 것이 좋을 듯.
• material이나 bootstrap은 기본적인 것을 컴포넌트 화 한다.

 

 

 

 

 

components/kanban/ChannelCategories.js

 

• render 메소드 내에서 콜백으로 넘기는 메소드를 클래스 컨스트럭터에서 바인딩하는 방식 대신에 요즘은 애로우 함수를 많이 쓰고 있음

handleSelectedChannel = (channelType, channelId) => { ... };

• 위에 autobind 해주는 것도 많음. ie. core-decorators
• props 를 localState 를 넘기는 것은 이유가 있어야 할 텐데요...
  • channelType 과 channelId 는 상위 페이지에서도 필요한 데이터임
  • 따라서 여기서 localState 로 관리하면 오히려 동기화하는 것이 더 복잡해질 듯 보임
  • 주로 localState 는 데이터와 상관없는 ui 상태 관리를 하기 위해서 사용되는 값임.
• componentDidMount 라이프사이클 매소드 자체가 async 가 아닌데, 여기서 async 를 쓰고 있음.
  • 데이터를 서버로 부터 가져오기 위한 코드때문에 어쩔 수 없이 사용한 듯 보임 
  • 큰 프로젝트에서 이런 구조 즉 컴포넌트 라이프사이클 메소드에서 axios API 호출을 직접 하는 것은 좋은 방식이 아닌 듯.
• channelCategoryInfo 에 '전체' 와 '선택' 채널에 추가 채널을 추가하는 코드가 비효율적임
  • 중복코드가 나오게 할 필요는 없어보임. 
  •  그냥 ...data 로 추가면 될 듯 보임

 

components/kanban/ChannelCategory.js

 

• Link 에 to 도 있고 onClick 도 있는 이유가 무엇인가요?
  • Routing 이 바뀌면 대부분 다시 렌더링이 들어가야할 텐데요.
  • Link 로 처리할 부분과 그렇지 않는 부분이 명확하게 구분되어야 할 필요있음.
• 이런식의 조건 기술은 프로그램 이해를 너무 어렵게 만듭니다.

this.props.channelType === channel.channelId

• 컴포넌트 내에서 CSS 클래스 이름을 동적으로 붙이는 것은  좋아 보이지 않음.

const makeClassByChannelType = (channelType) => {   let className = "top ";   if (channelType === "join") {     className = className + "joined on";   ... }

• 채널 타입 선택 액션이 어떤 UX 인지 잘 모르겠음.

 

components/kanban/channelDetail.js

 

• 핸들러 함수 명령 규칙을 명확하게 했으면 함.
  • handleOnChange~~~
  • handleOnClick~~~
• 콜백 Props 명명 규칙을 명확하게 했으면 함.
  • - onChange~~~~
  • - onClick~~~
• await 를 쓰지 않아야 할 곳에 쓰고 있음

await this.addTasksBrief(tasksBriefResponse);

• setState 의 CallBack까지 걸어서 처리해야하는 이유가 있나요? 다른 핸들러에서도 다 이런식인데 궁금하네요...

onClickCategory = () => {   this.setState(     {....},     () => this.callTasksBrief()   ); };

• More 버튼을 누르면 Page 를 증가시키고, 다시 로딩하는 코드가 있음. 이 흐름 처리하는 것이 맞나요?
  • 일단 페이지 번호가 로컬 스태이트로 처리한 이유가 있나요?
    • 페이지 번호가 이 컴포넌트 내부에서 사용하는 개념이면 맞을 것 같음
    • 그런데, 모델에 페이지 번호가 저장되어야 한다면... 로컬스테이트 처리할 필요가 없을 것 같음.
      • 모델 확인 필요
    • 스크롤 포지션도 저장되어야 할 것 같은데 페이지 대신에...
  • 이런 흐름은 어떤가요?
    • 다음 페이지를 요청하고,
    • 해당 데이터가 모델이 반영되고
    • 단순히 이를 화면에 그려주면 될 것 같음

  - 

 

components/common/PopupWrapper.js

 

• 이 프로젝트에서는 가능하면 컴포넌트와 서비스 호출은 분리했으면 합니다.
  • 말그대로 Props 로 표시할 데이터를 받아서 그려주면 될 형태가 일반적일 듯 보임.
• 다른 컴포넌트를 감싸는 구조의 컴포넌트로 처리하는 것이 맞는지 고민할 필요가 있음
  • 쓸만한 다이얼로그 컴포넌트 혹은 코드를 찾을 수 없었는지
• 다른 컴포넌트를 감싸는 방식도 이렇게 함수로 구성하는 방식이 아닌 보통 children props 나 커스텀  props 로 넘기는 것이 일반적임.
• 그리고 Popup 상태 역시 안에서 PopupWrapper 자체에서 컨트롤할 것이 아니라 외부에서 그릴지 말지를 결정하는 것이 좋을 것 같음.
  
  • x 버튼을 누른 경우 부모의 팝업 상태를 업데이트하게.
• UserPopup 같은 부분도 따로 컴포넌트로 떼내서 정의했으면 좋겠음.
• 팝업 처리 흐름은 이렇게 하면 어떨까?
  
  • 이벤트가 발생하면 커맨드(액션)를 호출하고
  • 커맨드를 실행하면
    • 조회 서비스가 호출되고
    • 내부적으로 관련 모델이 업데이트되고 
  • 커맨드가 완료되면(비동기) 그 상태에서 팝업 상태 모델을 업데이트
  • 그럼 화면에 팝업이 생성됨.
• 뷰와 뷰가 서로 의존성이 있을 때, 어떻게 제어할 지 고민이 필요함.
  • 부모-자식 관계인 경우
    • 부모에 자식의 뷰 상태에 관련된 상태에 존재하며, 이를 모델과 바인딩할지 아니면 로컬상태로 갖고 있을 지 고민해야 함.
    • 데이터 바인딩을 부모 뷰와 자식뷰 각각에 걸어야 하는지.... 어느 레벨까지 해야할 것인지?
      
      

components/common/CalendarPicker.js

 

• 데이트 픽커는 JQuery 를 사용하는 구조인가요?
  • 퍼블리시하는 쪽에서 확정된 방식인가요?
  • React 에서 JQuery 를 사용하는 방식이 이런 방식인가요?
• 캘린터 컴포넌트에서 기본적으로 사용될 콜백 Props 등이 보이지 않는 것 같습니다.
  • 아직 구현 중이라서?
• React 쪽에 Dom 을 직접 Reference 할 수 있는 방법도 있습니다.
  
  • React.createRef()

 

components/myChannel

• 공통에 들어갈 컴포넌트들이 여기서 많이 정의된 것 같은데요?
  • ButtonClose, InputRadio
  • 공통 컴포넌트를 정의하고, style 로 커스터마이징할 수 있는 구조로 갔으면 합니다.
• 왜 이름이 다 Pop 으로 시작되는지요?

 

components/myChannel/PopRightItem.js

 

• className 을 쓸 것인지 classnames 를 이용한 classNames 를 쓸 것인지 정해야 함.

 

 

mobx 적용 관련 구조

 

• mobx 단점은 무엇인가요?
  • 작은 프로젝트는 OK 지만 큰 프로젝트에서는 오히려 독이 될 수 있다
• mobx 멀티플 스토어
  • https://mobx.js.org/best/store.html
  • 도메인 상태를 저장하는 스토어와 UI 상태를 저장하는 스토어를 분리
  • 도메인 스토어는 도메인 오브젝트를 관리하고, 백엔드와 연동 등등..
  • 스토어를 트리형태로 구성
• mobx 세미나 부탁 - https://mobx.js.org/README.html
• mobx  Provider/inject  대신에 React Context 를 쓰라고 가드하고 있음.
  
  • Note: usually there is no need anymore to use Provider / inject in new code bases; most of its features are now covered by React.createContext.
  • https://mobx-react.js.org/recipes-context
• 타입 스크립트 vs PropTypes
• 스토어 부분은 테스트 코드도 작성했으면 좋겠습니다. 시간나면