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

[team-22][FE] 2주차 2번째 PR - Park + BB #205

Open
wants to merge 8 commits into
base: team-22
Choose a base branch
from

Conversation

healtheloper
Copy link
Collaborator

@healtheloper healtheloper commented Apr 15, 2022

💻 데모

링크

  • API 테스트는 Swagger 의 api-docs 를 통해 테스트해보실 수 있습니다.

🧐 어떤 생각으로?

  • handler 를 통해 상태관리하는 로직을 일관성 있게 바꿔보자.

    • handle + eventType + componentsName
      • 내부엔 상태변경 + 비즈니스 로직
  • PR 리뷰 사항 개선

    • Webpack alias
  • 요구사항 기능 구현

    • 드래그 앤 드랍 각자 구현해보기
      • 둘 다 드래그만 구현되어서 각자의 숙제로..남았습니다.
    • 카드 수정 기능, 새로운 카드 등록
      • 서버에 body 데이터 만들어 요청 보내기

👍 잘했던 || 좋았던점

useRef

  • Component 내부의 코드 순서가 너무 뒤죽박죽 이고 핸들러에서 어떤 요소를 써야 할 때 그 요소의 순서에 의해서 직접 사용이 불가능한 경우 어쩔 수 없이 DOM APIquerySelector 를 사용할 수 밖에 없었습니다.
  • 이런 문제를 어떻게 해결할 수 있을까 싶어서 처음에는 let 으로 전역에 임시로 선언한 후 재할당 하는 식의 아이디어를 떠올렸었는데, let 으로 재할당하는 방식을 최대한 지양 해보고자 했습니다.
  • 파크가 useRef 라는 것을 사용한다고 알려주어서 useRef 를 구현해보고 그것을 직접 사용하여서 내부의 코드 순서를 합의하에 정하여 변수 선언 - 핸들러 - 엘리먼트 의 흐름을 가져가 가독성을 높일 수 있었습니다.

Modal 분리과정

  • 이전 PR에서 Modal의 표시여부가 Card의 삭제 버튼을 눌러서 모달이 호출된다는 이유만으로 Card에서 그 부분을 핸들링하였고, Modal 을 열거나, 닫는 로직이 DOM API 에 의존해 있는 등 많은 문제가 있었습니다.
  • useRef의 구현으로, 해당 이벤트 핸들러들이 DOM API 에 의존하지 않도록 할 수 있었고, 해당 핸들러는 구조적으로 Card 보다 더 상위 컴포넌트에 위치하는 것이 더 좋을 것이라는 빰빰의 피드백에 힌트를 얻어 리팩토링을 진행했습니다.
  • 미션을 진행하면서 만들어 두었던 useState를 활용하는 방법으로 Modal의 표시 여부 현재 선택된(삭제여부를 묻고있는) 카드의 정보 를 저장해 둘 수 있어서 이전보다 좋은 로직을 만들 수 있었습니다.

🧐 질문

Webpack alias 설정

  • Components 안에서 다른 Components 를 참고할 때에는 상대경로로 하는게 경로가 더 짧아지기도 하고, 같은 Components 폴더라는 느낌이 있어서 상대경로로 하는게 더 좋지 않을까? 란 생각이 들었습니다.
    1. 일관성을 위해 resolve 된 경로부터 하는게 좋을지
    2. 같은 폴더(components) 인 경우 상대경로로 하는게 좋을지

handleClickButton ?

  • 만약 CloseButton 과 ConfirmButton 이 같은 로직만 처리한다고 했을 때, handleClickButton 이라는 공통된 함수를 사용하는게 좋을 지, 아니면 각 컴포넌트별 핸들러 함수를 선언해서 쓰는게 좋을지 고민되었습니다.
  • 하나로 썼을 때에 메모리적 효율을 가져갈 수 있겠지만 나중에 각각의 핸들러가 다른 로직을 처리할 수 있다는 가정하에 분리하는게 더 좋지 않을까 란 생각을 했습니다
const toggleClass = ($element, className) => {
    $element.classList.toggle(className);
};

//

const handleClickButton = ({target}) => {
    const $element = target.closest('button');
    toggleClass($element , styles.active);
};

// OR

const handleClickConfirmButton = () => {
    const $confirmButton = confirmButtonRef.current;
    toggleClass($confirmButton , styles.active);
};

const handleClickCloseButton = () => {
    const $closeButton = closeButtonRef.current;
    toggleClass($closeButton , styles.active);
};

BB-choi and others added 8 commits April 14, 2022 15:11
* feat: column 제목 영역  +버튼,x버튼 클릭 이벤트

* fix: x버튼 클릭 이벤트 삭제 및 취소 이벤트

* refactor: button요소 tagComponents이용하여 생성

* refactor: 템플릿 리터럴 관련 변수명 변경, 코드 분리

* refactor: 중복함수 제거

- on~~MouseOver, Out과 같은 함수를 handle~과 같이 수정

* refactor: 각 버튼마다 존재했던 handle함수를 하나로 합침

* refactor: 칼럼제목 + 버튼 클릭시 $cardWritable 표시

- 기존 칼럼 헤더에 있던 cardWritable을 Cards내부로 이동
- 클래스 toggle로 추가 / 제거

* fix: 취소 버튼으로 newCard 닫히지 않던 문제 해결

* refactor: 메서드명의 get~을 create로 변경
* remove: 사용하지 않는 파일 삭제

* refactor: 상수 -> 대문자로 사용

* refactor: LOG_TYPE 대문자 변경에 따른 uppercase

* refactor: Content components props

* refactor: tag, innerHTML -> template 변수명 변경

* refactor: useRef & DOM API 사용 로직 제거

* refactor: tag, innerHTML -> template 변수명 변경

* refactor: create column element refactor
* feat: date utils & get date diff

* feat: create getISODateDiff
* fix: webpack devtoom 설정 반대로되어있던 것 수정

* style: console 삭제
* feat: update card use double click

* style: input value 없을 때 등록버튼 비활성화

* refactor: inline 으로 css 조작하던 방식 리팩터링

* style: confirm button 안쓰는 클래스 삭제
* refactor: 불분명한 함수명, 변수명 변경

- 카드 등록 버튼 $addButton으로 변경
- 칼럼header의 +, x버튼 hover 핸들러 handleButtonHover로 변경

* feat: newCard 생성시 등록버튼 비활성 스타일

* feat: post 새로운 카드 등록하기

* fix: 처음 $newCard가 표시되면 submit 버튼 disabled처리

* fix: Fix a typo

* refactor: 함수명, 변수명 등 불확실한 이름을 변경

- id -> columnId
- putTodo -> createTodo

* fix: handleKeyUpInput에 빠졌던 조건 추가

- else if()의 조건에 !isInputEmpty 추가

* refactor: plusButton, deleteButton 핸들러 리팩토링

- xButton, deleteButton 이름이 혼재 되어있던 것도 deleteButton으로 통합

* refactor: handleNewCardVisibl~을 toggleCardVisible로 변경

* refactor: $plusButton 클릭 이벤트 핸들러 추가

- $plusButton이벤트 핸들러간 네이밍 통일 handle~PlusButton
* refactor: Cards.js에서 todos 정렬 함수 외부로 분리

- dateUtils 사용하여 분리

* refactor: Card.js 불필요한 함수 삭제

- onMouseOver, onMouseOut 등 불필요한 함수 삭제

* refactor: modal 핸들러 함수 위치 변경

- App에서 state로 관리하도록 변경
    - columns를 App에서 생성하므로 App에서 state로 관리
    - 선택된id를 state로 관리하므로 re-rendering으로 인해 클래스 토글이
      아닌 state로 모달 표시 여부 결정

* feat: 카드 삭제 / 모달 팝업 닫을 때 선택된 id 리셋

* fix: x 버튼 hover시 cursor 범위 오류 해결

* fix: PR confilct 해결

* refactor: SideContent 최신순 정렬

* refactor: PR리뷰 반영
* refactor: visible 처리 로직, input 값 체크 로직 추가

* env: webpack alias 설정
@healtheloper healtheloper added the review-FE New feature or request label Apr 15, 2022
@ppamppamman
Copy link

안녕하세요! 리뷰를 드리기 전에 오늘 발표이신것 같아서요 질문에 대한 답변만 먼저 짧게 달아둘게요!

Webpack alias 설정
Components 안에서 다른 Components 를 참고할 때에는 상대경로로 하는게 경로가 더 짧아지기도 하고, 같은 Components 폴더라는 느낌이 있어서 상대경로로 하는게 더 좋지 않을까? 란 생각이 들었습니다.
일관성을 위해 resolve 된 경로부터 하는게 좋을지
같은 폴더(components) 인 경우 상대경로로 하는게 좋을지

저는 상대경로는 무조건 지양하는 편입니다. 읽는 경우에 "상대경로니까 이것은 이 컴포넌트에서만 쓰이는 거겠구나." 라고 생각이 안되더라구요. 아, index.js나 index.ts처럼 index파일이 컴포넌트의 import export의 엔트리포인트로 쓰이는 경우에는 상대경로로 쓰는 것 같아요.

handleClickButton ?
만약 CloseButton 과 ConfirmButton 이 같은 로직만 처리한다고 했을 때, handleClickButton 이라는 공통된 함수를 사용하는게 좋을 지, 아니면 각 컴포넌트별 핸들러 함수를 선언해서 쓰는게 좋을지 고민되었습니다.
하나로 썼을 때에 메모리적 효율을 가져갈 수 있겠지만 나중에 각각의 핸들러가 다른 로직을 처리할 수 있다는 가정하에 분리하는게 더 좋지 않을까 란 생각을 했습니다

닫는 버튼과 확인 버튼이 일반적으로는 같은 로직을 처리하지는 않겠죠? 다만, 예를 들어 확인버튼과 종료버튼이 '모달을 끄는 행위'를 똑같이 수행한다면, '모달을 끄는 행위'에 집중해서 굳이 handle prefix 붙이지 않고 closeModal 이라고 이름 지을 것 같습니다. handleClickButton은 너무 제너럴한것 같구요 styles.active를 달아주는 toggle을 해주는 거니까 활성상태전환 -> toggleActive(or activate)Status 뭐 이런식으로 하지 않을까 싶어요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review-FE New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants