코드리뷰 4

routes/MyChannel.js

 

 

 

pageRegist = () => {   const { history } = this.props;   history.push("/myChannel/regist"); };

• 메소드는 명령형으로 작성했으면 합니다.
  
  • pageRegist => routeToChannelRegistrationPage
• history 에서 이렇게 직접 Push 할 필요가 있나요? <Link> 사용하면 될 텐데요.
  • <Link to="myChannel/regist"><FilIconButton...></Link>
  • 위처럼  가능할 줄 알았는데, 그렇지 않다고 하네요. <a>  태그 안에 <button> 태그는 들어갈 수 없다고...

 

render() {   return (     <>       <HashRouter>          <Switch>            <Route              path="/myChannel/list"              component={() => (               <>                 <HeaderAside>                 <HeaderUser UserName="김지혜" />                 <FillIconButton                     ButtonTitle="채널 등록"                     onClick={this.pageRegist}                 />                 </HeaderAside>                 <MyChannelMain />            </>           )}        />        ...

• <HashRouter> 는 상위에도 하나 있을 텐데, 여기에 하나더 추가해서 중복해서 쓸 필요가 있나요?
  
  • 혹시 이유가 있나요? <HashRouter> 없이 <Route> 를 사용되는 것을 막기 위한  가드 코드?
• 라우터 사용자 메뉴얼에서 보면 component 에 inline function 을 사용하면 매번 컴포넌트를 새로 생성하여 성능 저하가 있다고 언급하고 있습니다
  • render 나 children prop 을 사용하라고 나와 있습니다.
  • 기본적 라우터 컴포넌트에서 이렇게 컴포넌트를 구성하지 말고, 이미 구성된 컴포넌트를 지정했으면 합니다.
• onClick 에 굳이 메소드를 만들어서 매핑하지 말고 간단하게 ()=> history.push(...) 를 직접 써도 상관없을 듯 보입니다.

 

 

/myChannel/list /myChannel/regist /myChannel/modify/:channelId /myChannel/:channelId

• 내부 routing 규칙도 정해야 할 듯 보입니다.
  
  • 라우팅에서 대문자를 잘 안 쓴다고 합니다.
    
    • myChannel => my-channel
    • 언더바(_) 말고 대시(-) 사용
  • REST 형식으로 사용하는 것이 제일 무난해 보입니다
    
    • my-channel/channels
    • my-channel/channels/:channelId
    • my-channel/channels/:channelId/modify
    • my-channel/channel-registration
  • REST 설계할 때 레벨 개념을 사용했으면 합니다.
    • :id 경로가 들어갈 곳에는 다른 상수 경로를 쓰지 않습니다.
•  

 

routes/Post.js

 

• <Route> 에서 컴포넌트 지정 이슈
• <Route> 에서 component prop 사용 방식
  • component = { () => <AddPost> }
• 객체 변수는 소문자로 시작
  • RootStore

 

handleRegisterBtn = () => {   const { history } = this.props;   const { RootStore } = this.props;   const { PostStore } = RootStore;   const { selectedChannel } = PostStore;   const { channelId } = selectedChannel;   if (!channelId) this.handlePopupToggle();   else history.push(`/post/regist/${channelId}`); };

• 코드 최적화는 항상 고민해주세요....
  
  • const { history, rootStore} = this.props;
  • const channelId = rootStore.post.selectedChannel?.channelId;

 

{isPopOn && (   <PopSelectWrapper onPopClose={this.handlePopupToggle} /> )}

• PopSelectWrapper 는 전혀 의미를 추론할 수 없는 컴포넌트 이름입니다.
  
  • 컴포넌트로 만들던지 아니면
  • children prop 을 사용하여 안에 내용을 기술하게 하던지 둘중에 하나는 사용해야할 것 같습니다.
  • 따로 이런 것을 만들지 말고 antd 의 Modal 을 사용하는 것은 가능한가요?

 

/post/list /post/regist/:channelId /post/regist/:postId /post/:postId

• REST
  • post/posts
  • post/posts/:postId
  • post/task-registration?channel-id=:channelId
  • post/message-registration?channel-id=:channelId

 

routes/MemberSelect.js

 

 

<FillButton   ColorClass={"green"}   SizeClass={"size-default"}   ButtonTitle={"취소"} />

• props 는 camel 형식으로 했으면 합니다.
  
  • ColorClass => colorClass
• 그냥 antd 의 <Button> 을 그대로 사용했으면 합니다.
• 새로운 컴포넌트를 만들 때 꾸미기 위한 속성을 따로 prop 으로 만들어  넘겨주는 것이 아니라...
  •  className 을 지정하거나 ID 를 지정해서 css 단에서 스타일 입히면 좋겠습니다.
• FillButton 이 components/common/widget 에 있던데, 이 디렉토리의 용도는 무엇인가요?

 

components/post/AddMessagePopupWrap.js

• Popup 이면 Popup 이지 왜  PopupWrap 라고 이름을 짓나요?
• AddMessagePopup 을 구성하는데 너무 많이 컴포넌트를 쪼갠 것 아닌가요?
  • AddMessagePopupWrap
    • AddMessagePopupBox
      
      • AddMessagePopupTop
      • AddMessagePopupBottom
        
        • AddMessagePopupSearch
        • AddMessagePopupItem
          • CheckboxForm
• 다이얼로그는 그냥 antd 를 사용하면 안될까요?
  • 퍼블리셔 문의 필요

 

 

const popupStyle = {   display: "block",   zIndex: "100",   transform: "scale(1)" };

• 컴포넌트 안에서 인라인으로 스타일을 먹였는데, 이유가 있을까요?
  • 특별한 이유가 아니면 인라인 스타일링은 지양할 필요가 있을 것 같습니다.

 

components/post/AddMessagePopupItem.js

 

 

<img src={require('../../resources/images/default_profile.png')} alt="" className={"thumbnail"} />

• require 를 안쪽에 쓰는 것은 아닌 듯 보입니다.

 

 

components/post/PostView.js

• <main> 태그는 어떤 의미인가요?

 

components/post/PostMain.js

• <BoxItem> 처럼  컴포넌트 이름을 짓지 말고 <PostListItem> 처럼  의미를 파악할 수 있게 했으면 좋겠습니다.

 

PostMain.wrappedComponent.propTypes = {    // eslint-disable-next-line react/forbid-prop-types    RootStore: PropTypes.object.isRequired, };

• PostMain.wrappedComponent.propTypes 를 사용하는 것은 어떤 의미인가요?
  
  • inject 된 스토어는 이런 식으로 기술해야하나요?
  • https://github.com/mobxjs/mobx-react#using-proptypes-and-defaultprops-and-other-static-properties-in-combination-with-inject

 

setCalendarFilter = (startDate, endDate) => {   const { RootStore } = this.props;   const { PostStore } = RootStore;   PostStore.setPostSearchDate(startDate, endDate);   // this.callTasksBrief(page, false, startDate, endDate); };

• 디콤포지션은 두번할 필요가 있었을까 싶네요.
  • 굳이 하겠다면
  • const { RootStore: { PostStore } } = this.props
• SearchDate 를 Store 에 저장한 이유가 있나요?  
  • 이 설정을 다른 컴포넌트에서 재사용하지는 않을 것 같고
  • 이 설정이 LocalStore 에 저장되어 사용할 것 같지도 않은데요
  • 이런 경우를 위해서 컴포넌트의 local state 가 사용됩니다.
  • this.setState( { searchStartDate: startDate, searchEndDate: endDate } );
    • 저장된 state 는 검색을 클릭할 때 액션의 아귀먼트로 넘어가게 됩니다.(다음 코드 리뷰 참조)

searchPosts = (searchWord) => {   const { RootStore } = this.props;   const { PostStore } = RootStore;   PostStore.setSearchWord(searchWord);   PostStore.resetRowNum();   PostStore.getPosts(); };

• 이벤트 핸들러 함수인데. 이름이 searchPosts 보다도 'handleClickSearchBtn' 형태가 좋아 보입니다.
  • 컨벤션 통일 필요
• 액션을 3개로 나누어서 전달하고 있는데, 하나로 통합할 수 있을 것 같습니다.
  
  • 우선 앞서 선택한 부분에서는 local state 에 저장하고
  • 여기서는 이런 식으로 
    • const {startDate, endDate, ... } = this.state;
    • PostStore.getPosts({ startDate, endDate } )
  • 스토어 각각의 조건을 저장하는 식으로 가면 조건 리셋도 들어가야 해서 번거러울 것 같습니다.

 

moreItem = () => {   const { RootStore } = this.props;   const { PostStore } = RootStore;   PostStore.increaseRowNum();   PostStore.getPosts(); };

• 이런식으로?
  • const {startDate, endDate, ... } = this.state;
  • PostStore.appendPosts( { startDate, endDate, pageSize: 10 ,... } );
  • 혹은 
  • const { startDate, endDate, ... }  = this.state;
  • const { RootStore } = this.props.
  • const postCount  = RootStore.post.currentPostCount;
  • PostStore.getPosts( { startDate, endDate, postCount: currentPostCount + 10 } );  

 

<BoxItem   postId={post.postId}   bookmarkYn={post.bookmarkYn}   channelId={post.channelId}   channelName={post.channelName}   ...

• 왜 이렇게 props 로 분리하고 있나요? 그냥 post props 로 넘기면 되는데요.

 

 

<SelectBox   items={["전체", "읽음", "안읽음"]}   default="전체"   setSelected={this.setPostReadSelected} />

• 이벤트 콜백 prop 이름은 on~~~ 로 시작했으면 합니다.
  • onSelect
  • onClick
  • onChange
  • ...

 

<MoreItemButton moreItem={this.moreItem} />

• 이벤트 콜백 prop 네이밍 컨벤션

 

components/posts/PopSelectWrapper.js

• 컴포넌트 이름에 적당한 의미를 포함하는 이름으로 했으면 함. Wrapper, Pop... 은 UI 적인 그리고 구현적인 의미밖에 없음.

 

const { RootStore, onPopClose } = this.props;

• onPopClose 이벤트 콜백 prop 네이밍 컨벤션에 맞추었으면 합니다.

 

 

<Categories   onPressOpenBtn={() =>     this.handleCategoryToggle(channelItems.channelType) }

• Press 보다 Click 이 많이 사용되는 듯 보입니다.
  • onPressOpenBtn => onClickOpenBtn
• "Open"  버튼을 눌렀는데, 카테고리가 토글(handleCategoryToggle)? 의미가 와닫지 않음

 

• 컴포넌트 트리 구조가 잘 안그려짐.
  • PopSelectTop
    
    • CheckBoxForm
    • SearchBox
      • SearchFrom
      • SmallCloseButton  => 왜 Search Box 안에 있나요?
  • PopSelectBottom
    • Categories
      
      • CheckBoxForm  => 왜 nested 구조로 갔는지 잘 알 수 없음.
    • PopSelectBoxItem 

 

 

<PopSelectBoxItem   key={channel.channelId}    channelId={channel.channelId}   channelName={channel.channelName}   createUserId={channel.createUserId}   createUserName={channel.createUserName}   ...   onCheckBoxChecked={() =>     this.handleCheckBoxChecked(       channel.channelId,       null,       channel.channelName    ) }

• 채널 정보를 나타내는 컴포넌트인데 이름이 <PopSelectBoxItem> 이면 이상함.
• 마찬가지로 개별 props 로 넘기는 것이 아니라 channel 통채로 넘기는 것이 좋을 것 같습니다.
• 컴포넌트 이벤트 콜백 네이밍 컨벤션
  • onCheckBoxChecked => onChange
  • 혹은 onCheckBoxChecked => onSelectChannel

 

 

components/myChannel/MyChannelMain.js

 

 

this.handleOpenBtnFav = this.handleOpenBtnFav.bind(this);

• 다른 방법으로 bind 처리해주세요

 

 

const chTypeList = [   { channelType: "join" },   { channelType: "private" },   { channelType: "follow" },   { channelType: "favorite" }, ];

• 채널 타입은 서버에서 가져오는 데이터 아니였던가요? 이렇게 하드코딩하면 안될 것 같습니다.

 

 

handleOpenBtnFav = () => { const { isOpenFav } = this.state; this.setState({ isOpenFav: !isOpenFav }); }; handleOpenBtnJoin = () => { const { isOpenJoin } = this.state; this.setState({ isOpenJoin: !isOpenJoin }); }; handleOpenBtnPriv = () => { const { isOpenPriv } = this.state; this.setState({ isOpenPriv: !isOpenPriv }); }; handleOpenBtnFlw = () => { const { isOpenFlw } = this.state; this.setState({ isOpenFlw: !isOpenFlw }); };

• 채널 타입이 데이터면 이렇게 하드 코딩해서 각각의 핸들러를 구현할 것이 아니라 하나의 타입을 받아서 처리하는 방식으로 구현해야함.

 

 

<Categories onPressOpenBtn={this.handleOpenBtnFav} channelIcon="favorite-title" channelType="즐겨찾기 채널" /> ... <Categories onPressOpenBtn={this.handleOpenBtnJoin} channelIcon="joined-title" channelType="참여중 채널" /> ... <Categories onPressOpenBtn={this.handleOpenBtnPriv} channelIcon="private-title" channelType="프라이빗 채널" />

• 이런식으로 각각을 따로 컴포넌트화 하여 추가할 것이 아니라 채널 타입에 대해서 루프를 돌려서 처리해야할 듯 보임

 

 

isEmpty = (array) => { return array == null || array.length === 0; };

• 컴포넌트 안에 구현해야할 내용은 아니고 Util 에 구현해야할 내용인듯 보임
  • 언더스코어를 써도 됨

 

 

• 컴포넌트 트리 구조 문제 - Categories 와 TaskBoard 레벨이 같음.
  
  • 이런 구조로 되어야 함
    • Categories
      • TaskBoard 

 

components/common/TextSymbolLink.js

• 왜 PostStore 랑 연계되어 있는 것이 공통에 들어와 있나요?
•  그리고 단순 컴포넌트를 Store 랑 연계시키지 말로 Prop 로 콜백을 전달 받아서 바깥에서 처리해야할 것 같습니다.