[Team 17][FE] Todo List 2주차 PR1 #141

ju-kkim · 2022-04-13T07:50:25Z

team-17 프론트엔드

안녕하세요~ 이든, J 입니다.
MVVM을 사용해보기로 하고 Model. View 분리를 우선으로 생각하기로 했는데 구현하다보니 MVC가 되어버렸습니다...😅

협업과정

이번 작업은 서로 분업하여서 작업했습니다.
분업을 하다보니 서로 코드 스타일이 다른부분이 있습니다. 이부분은 대부분의 기능 구현이 완료된 후 시간이 된다면 리팩토링 진행해보려고 합니다.
참고해서 리뷰 부탁드립니다.

J : todo_container 렌더, 카드 추가, 카드 수정
이든 : 카드이동 (드래그 앤 드롭), 카드 삭제

👉🏻 카드이동 기능은 아직 완료 되지 않고 구현중이라서 dragNdrop.js 파일에서 별도록 작업중입니다. 기능 구현 완료 후 파일 합칠 생각입니다.

👉🏻 현재 두 기능을 합치면서 클릭이벤트와 마우스다운이벤트가 겹쳐서 생기는 오류가 있어 app.js에서 dragNdrop.js 부분은 주석 처리 해놓은 상태입니다!

👉🏻 드래그 기능할때 스타일 깨지는 부분은 추후에 수정할 예정입니다.

진행사항

카드 추가
카드 삭제
카드 수정
카드 이동 (진행중)

기능 영상보기

기능 영상보기

카드추가 기능

addCard.mov

카드삭제 기능

default.mp4

카드수정 기능

editCard.mov

카드이동 기능

dragNdrop.mp4

고민 및 질문

각자 나눠서 기능 구현하다 보니 코드 스타일이 다른 부분에 대해서는 보통 미리 상의를 하고 진행해야 하는 건가요?
패턴분리나 메소드 나누는 방식, 네이밍 등 서로의 생각이 조금 차이가 있어서 이런부분을 함께 이야기를 많이 했습니다.
어디서부터 어디까지 상의를 하고 맞춰야 할지 어려운것 같습니다.

- menuStatus update 메서드 - menuStatus get 메서드

[FE] alert창 기본 동작 구현

- 수정하면서 생기는 render 문제 해결

[FE] 헤더기능구현 및 히스토리 애니메이션 기능

[FE] 드래그앤 드롭 기본 기능 구현(도착 잔상 및 맨끝에 추가 제외)

…nto FE/feature/#36-card_add

[FE] 카드 추가 기능 구현

- 카드 write 모드 스타일 추가 - 수정기능 구현하면서 생기는 오류들 해결

[FE] 카드 삭제 기능 구현

…nto FE/feature/#37-card_edit

[FE] 카드 수정기능 구현

kowoohyuk

안녕하세요!
이번 리뷰들 담당하게된 json입니다!
먼저 리뷰가 늦어진점 다시 한 번 사과드리겠습니다.. 🙏

우선 영상이 있어서 그런지 파악하는데 시간이 많이 단축 됐고, 덕분에 리뷰를 달기 정말 쾌적했어요! 감사합니다!!
그리고, 전체적으로 하나의 함수가 최대한 적은 기능을 담당해서 코드를 읽기 수월한 점도 👍 !

질문사항

각자 나눠서 기능 구현하다 보니 코드 스타일이 다른 부분에 대해서는  
보통 미리 상의를 하고 진행해야 하는 건가요?

생각하신대로 미리 상의를 하고 진행하면, 불필요한 시간소요를 방지할 수 있습니다!

패턴분리나 메소드 나누는 방식, 네이밍 등 서로의 생각이  
조금 차이가 있어서 이런부분을 함께 이야기를 많이 했습니다.

어디서부터 어디까지 상의를 하고 맞춰야 할지 어려운것 같습니다.

문서를 통해서 서로 합의를 하고, 그 문서를 기반으로 스타일을 맞춰나가면 좋을 것 같아요!

네이밍과 관련해서 추가를 add로 할지, create로 할지
들여쓰기는 몇칸으로 할지
싱글 쿼테이션이나 더블 쿼테이션 등...

이런 내용들을 서로간의 합의가 될 때까지, 하지만 너무 깊지는 않게 최대한 정리를 하고 시작하시면 훨씬 더 원활한 개발을 진행하실 수 있을거에요 😄

더불어 린트를 활용해보는 것도 좋겠죠 😉

kowoohyuk · 2022-04-14T09:52:58Z

frontend/src/js/Utils/createTemplate.js

+const createTagTemplate = (tag, content, className = '') => {
+  return `<${tag} class="${className}">${content}</${tag}>`;
+};


createTagTemplate의 파라미터에서 두 번째 값이 공백으로 오는게 빈번하게 보이는 것 같아요!
현재 PR내의 코드에서는 content는 falsy할 수 있지만 className은 truthy인 것 같은데 그렇다면 혹시 순서를 바꿔보면 어떨까요?

const createTagTemplate = (tag, className, content); // before createTagTemplate('main', '', 'main'); // after createTagTemplate('main', 'main');

순서를 바꾸는것이 훨씬 더 깔끔하고 실수를 줄일것 같습니다.

kowoohyuk · 2022-04-14T09:55:36Z

frontend/src/js/Utils/template.js

+          class="user_img"
+          style="background-image: url(./src/img/user.svg)"


user img를 담당하는 태그에 클래스 네임이 존재하는데
따로 인라인으로 이미지를 제어한 이유가 궁금합니다! ✋

혹시.. 임시일까요?!

로그인에 대한게 요구사항에 없는 부분이긴한데 사용자가 여러명일 경우를 생각하고 작성을 했습니다.
img 태그로 넣을 경우 사용자가 이미지 크기를 고려하지 않고 넣어도 백그라운드로 제어하면 어느정도 케어할수있기때문에 백그라운드로 처리했습니다!
제가 설명을 잘 한건지 모르겠네요..😅

제가 질문을 상세하게 드리지 못한것 같아요!
그런데 답변을 듣고 다시 한번 코드를 보니 내용이 이해되었습니다! 👀

궁금했던 부분은.. user_img라는 클래스네임이 있으니 인라인 스타일을 안써도 되지않을까 였었는데요!
말씀하신대로 다수의 사용자라면 이미지는 데이터로 제어하는 방향으로 가시겠군요!! 😄

kowoohyuk · 2022-04-14T10:05:15Z

frontend/src/js/dragNdrop.js

+  $newCard.style.position = 'absolute';
+  $newCard.style.zIndex = 1000;
+  $newCard.style.borderRadius = '6px';
+  $newCard.style.backgroundColor = '#fff';
+  $newCard.style.boxShadow = '0px 1px 30px rgba(224, 224, 224, 0.3)';
+  $newCard.style.width = '310px';
+  $newCard.style.height = '115px';


여러 종류의 스타일을 설정할 때,Object.assign() 이나 cssText 로 정리해도 괜찮을 것 같습니다!

kowoohyuk · 2022-04-14T10:16:16Z

frontend/src/js/Todo/Column/main.js

+    this.model = new ColumnModel(id, title, cardCount);
+    this.view = new ColumnView();


혹시 ColumnModel의 constructor는 파라미터를 받는데 ColumnView는 안받는 이유가 있을까요??
ColumnView도 ColumnModel과 동일하게 만든다면 init에 전달하지 않아도 될 것 같다는 생각이 들었습니다! 👀

id, title, cardCount 이 3개가 모델에 있는 상태값이라고 생각했고,
view의 역할에서 생각했을때 컨트롤러로 부터 받아서 사용을 해야할 것 같다고 생각했습니다.
Model, View 양쪽에 있어도 되는건가요?

제가 리뷰의 아키텍처 부분을 제대로 읽지 않았군요..! 😢
말씀대로 MVC나 MVVM같은 패턴을 염두해두고 작성하셨다면 말씀하신대로
분리하는 방향으로 가는게 좋을 것 같아요! 👍

kowoohyuk · 2022-04-14T10:17:51Z

frontend/src/js/Todo/Column/main.js

+  }
+
+  init({ id, title, cardCount }) {
+    this.view.render(id, title, cardCount);


ColumnView의 constructor에 render를 포함해보면 어떨까요?
따로 의도가 있으신지도 궁금합니다!

뭔가 작업하다가 init에 render말고 다른것들이 생길수도 있다고 생각했던것 같아요..ㅎㅎ
constructor로 옮길게요!!☺️

아! 그런 의도였었군요!
그렇다면 생각하신대로 확장 여부에 따라서 조정하는 방향도 좋을 것 같아요!

kowoohyuk · 2022-04-14T10:19:02Z

frontend/src/js/app.js

+import Controller from './Controller/Controller.js';
+import { Header } from './Header/main.js';
+import { History } from './History/main.js';
+// import { dragNdrop } from './dragNdrop';


~~불필요한 주석은 삭제...?~~

kowoohyuk · 2022-04-14T10:22:07Z

frontend/src/js/Header/Model.js

+  }
+
+  updateStatus() {
+    this.menuStatus ? (this.menuStatus = false) : (this.menuStatus = true);


this.menuStatus = !this.menuStatus;

는 어떨까요?! ;)

kowoohyuk · 2022-04-14T10:40:55Z

맞다! 깜빡했네요!

개발하느라 정말 수고하셨습니다! 👍

ju-kkim and others added 30 commits April 11, 2022 04:02

feat: [#33] Header Template 추가

40c24a3

feat: [#33] HeaderModel menu 상태 관련 메서드 추가

b911b18

- menuStatus update 메서드 - menuStatus get 메서드

feat: [#33] HeaderView render 기능 추가

fa8e3e5

feat: [#33] HeaderView 이벤트 등록 메서드 추가

95571ca

feat: [#33] HeaderModel, HeaderView 생성하는 class Header 추가

f53c18e

feat: [#33] menuBtn 클릭핸들러_menuStatus update하기

23728ee

feat: [#40] History Template 추가

46d06ff

feat: [#40] HistoryView render 기능 추가

d2776a8

feat: [#43] History closeBtn 이벤트 등록

746d751

feat: [#43] History animation 기능 추가

e1aba4a

feat: [#33][#43] menuBtn, closeBtn 클릭핸들러, 히스토리 애니메이션 호출

db4b666

feat: [#43] HistoryView 생성하는 class History 추가

44c0b9a

feat: Controller, Header, History 인스턴스 생성

1d952f5

fix: 기존 body 내부 요소 주석처리

7ba3508

feat: [#34] alert창 기본 동작 구현

8867027

refactor: utils폴더구조 수정

b9d6051

Merge pull request #47 from rxdcxdrnine/feature/#34-alert

f1a5553

[FE] alert창 기본 동작 구현

fix: 인스턴스생성할 필요 없는 클래스 객체로 수정

698bce6

- 수정하면서 생기는 render 문제 해결

refactor: [#34] 템플릿리터럴 코드 분리

ff53792

fix: git 충돌 해결

c3ebec5

Merge branch 'frontend' into feature/#33-Header

2b90a8b

Merge pull request #44 from rxdcxdrnine/feature/#33-Header

225733d

[FE] 헤더기능구현 및 히스토리 애니메이션 기능

fix: [#36] 카드 write모드 스타일 수정

70db253

feat: [#35] 컬럼 및 카드 템플릿 추가

e305ca8

feat: [#36] 카드추가 버튼 클릭 후 카드박스 노출 기능 추가

b6c47a5

feat: [#36] 카드추가 기능

25148c6

feat: [#36] 카드추가 취소 기능 추가

747e06f

feat: [#39] 드래그앤 드롭 도착 잔상 및 맨끝에 추가 제외한 기본 기능 구현

79cd48a

Merge pull request #59 from rxdcxdrnine/FE/feature/#39-drag_n_drop

4a39686

[FE] 드래그앤 드롭 기본 기능 구현(도착 잔상 및 맨끝에 추가 제외)

Merge branch 'frontend' of https://github.com/rxdcxdrnine/todo-list i…

05d650d

…nto FE/feature/#36-card_add

ju-kkim and others added 2 commits April 13, 2022 15:52

chore: 드래그기능과 카드추가기능 이벤트 충돌로 인한 주석

8f6e05e

Merge pull request #57 from rxdcxdrnine/FE/feature/#36-card_add

b6c9bec

[FE] 카드 추가 기능 구현

ju-kkim added the review-FE New feature or request label Apr 13, 2022

ju-kkim requested a review from choisohyun April 13, 2022 07:50

ju-kkim assigned choisohyun Apr 13, 2022

ju-kkim requested review from kowoohyuk and removed request for choisohyun April 13, 2022 09:23

ju-kkim assigned kowoohyuk and ju-kkim and unassigned choisohyun and ju-kkim Apr 13, 2022

ju-kkim and others added 8 commits April 14, 2022 01:14

feat: [#37] 카드수정 기능 추가

40e841a

- 카드 write 모드 스타일 추가 - 수정기능 구현하면서 생기는 오류들 해결

chore: 지우지 못한 주석 삭제

61b1762

feat: [#38] 카드 삭제 기능 구현

8c3ad49

fix: [#37] cardView의 remove 메서드 사용

e9b299b

Merge pull request #73 from rxdcxdrnine/FE/feature/#38-deleted_card

8deb551

[FE] 카드 삭제 기능 구현

Merge branch 'frontend' of https://github.com/rxdcxdrnine/todo-list i…

55d535b

…nto FE/feature/#37-card_edit

feat: [#38] 카드삭제 버튼 mouseover, mouseout이벤트 추가

2c63632

Merge pull request #67 from rxdcxdrnine/FE/feature/#37-card_edit

b1e7253

[FE] 카드 수정기능 구현

kowoohyuk approved these changes Apr 14, 2022

View reviewed changes

kowoohyuk merged commit 28b0de7 into codesquad-members-2022:team-17 Apr 14, 2022

		class="user_img"
		style="background-image: url(./src/img/user.svg)"

		this.model = new ColumnModel(id, title, cardCount);
		this.view = new ColumnView();

[Team 17][FE] Todo List 2주차 PR1 #141

[Team 17][FE] Todo List 2주차 PR1 #141

Uh oh!

Conversation

ju-kkim commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

team-17 프론트엔드

협업과정

진행사항

고민 및 질문

Uh oh!

kowoohyuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ju-kkim Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kowoohyuk Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kowoohyuk commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ju-kkim commented Apr 13, 2022 •

edited

Loading

ju-kkim Apr 14, 2022 •

edited

Loading

kowoohyuk Apr 14, 2022 •

edited

Loading

kowoohyuk commented Apr 14, 2022 •

edited

Loading