[1주차] 강다혜 미션 제출합니다. #5
Conversation
Feature/setup: set up project
Feature/basic layout: make HTML structure
Feature/class definitions: define subject / task class
Feature/add subject: add subject
Feature/delete subject: subject removal function
Refactor/class private properties
Feature/toggle state
Refactor/codebase
과제 제출용 branch로 merge
There was a problem hiding this comment.
사실 코드 리뷰를 하면서, 제가 남겨드린 팁보다 배운 것이 훨씬 많았던 기회였습니다....😙
기존에 디자인 패턴을 공부하면서 mvc, mvvm 같은 내용을 개념적으로만 접했는데 실제로 남의 코드에서 이렇게 명확하게 구현된 것을 본건 처음이었습니다.
기본적인 html 문서의 파싱 동작 방식과 ES6에 추가된 Javascrtipt 클래스 문법을 이렇게 유려하게 활용하시는 모습은 마치 프론트엔드보다 Java 스프링 부트 프레임워크를 보는 듯했어요.
오히려 제가 더 공부가 된 것 같아 너무 감사드리고 앞으로 종종 재밌는 주제에 대해서 토론할 수 있었으면 좋겠습니다. 이버 과제 너무 수고 많으셨어요 👍
| :root { | ||
| --background: #121212; | ||
|
|
||
| --open-column-background: #31627e1a; | ||
| --in-progress-column-background: #7267451a; | ||
| --done-column-background: #2a5f2d1a; | ||
| --open-column-border: #0d5580; | ||
| --in-progress-column-border: #785e17; | ||
| --done-column-border: #0b5c18; | ||
|
|
||
| --subject-background: #121212ee; | ||
|
|
||
| --text-color: #ffffff; | ||
| --cancel-text-color: #505050; | ||
|
|
||
| --border: #505050; | ||
| --button-border: #ffffff; | ||
| } |
There was a problem hiding this comment.
variable.css파일을 만들어주시고, 필요한 css 파일 여기저기에서 var()을 활용하여 시스템화 해주셨네요!
순수 css 만으로도 tailwindCSS의 디자인 시스템화 같은 방식을 볼 수 있어 좋았습니다.
| <link rel="stylesheet" href="css/kanban.css" /> | ||
| <link rel="stylesheet" href="css/subject.css" /> | ||
| <link rel="stylesheet" href="css/task.css" /> | ||
| <link rel="stylesheet" href="css/variables.css" /> |
There was a problem hiding this comment.
저도 궁금한 것이 있어 여쭤봅니다! 현재 여러 css 파일에서 사용되는 variable.css 파일이 가장 마지막 순서로 로드되고 있네요! 현재 프로젝트가 너무 잘 작동하고 있는데, 직관으로 생각해보면 마지막에 로드 되는 것이라 다른 css 파일에서 css 변수를 가져다 쓸 때 제대로 동작하지 않을 수도 있겠다는 생각이 듭니다.
혹시 이를 어떻게 해결하신지, 또는 어떻게 괜찮은지 설명해주실 수 있나요?
There was a problem hiding this comment.
오잉.. 그냥 variable.css 파일을 나중에 만들어서 맨 뒤에 추가하게 되었는데요, 생각해보니 그런 문제가 있을 수 있겠네요😨
그래서 여러 군데 찾아보다가 결국 chatgpt한테 물어봤더니
CSS 변수(--variable-name)는 스타일시트의 파싱 중에 수집되며, 실제 값이 필요한 순간에만 참조됩니다. CSS 변수는 문서 전체의 파싱이 완료된 후 참조되기 때문에, 스타일시트 내 어디에서든지 사용이 가능합니다. 따라서 변수 정의가 앞서 있는지, 뒤에 있는지는 중요하지 않으며, 이는 브라우저가 CSS를 파싱하고 변수의 값을 나중에 참조하기 때문입니다.
라고는 하는데 적어준 출처에서는 그런 내용을 확인할 수 없었어요...! 지금은 시간이 없어서 다른 리뷰 다 보고 다시 돌아와서 확실히 알게 되면 다시 코멘트 드릴게요!
| <link rel="stylesheet" href="css/kanban.css" /> | ||
| <link rel="stylesheet" href="css/subject.css" /> | ||
| <link rel="stylesheet" href="css/task.css" /> |
There was a problem hiding this comment.
스타일링을 전반적인 칸반, 목표를 담은 subject, subject 안의 각각의 아이템인 task 로 나누어서 모듈화를 해주셨네요! 👍
| <script src="js/index.js"></script> | ||
| <script src="js/models/subject.js"></script> | ||
| <script src="js/models/task.js"></script> | ||
| <script src="js/viewModels/subjectViewModel.js"></script> | ||
| <script src="js/viewModels/taskViewModel.js"></script> | ||
| <script src="js/constant.js"></script> | ||
| <script src="js/utils/index.js"></script> |
There was a problem hiding this comment.
html 문서가 parsing 되는 순서를 고려하여 js 파일을 body 태그 다음에 위치하여 모든 DOM 요소가 로드된 이후 js가 동작하도록 만들어주셨네요!
이 방법도 너무 좋고, script 태그의 defer 속성을 이용하시면 head 태그 내에 script 요소를 배치해도 될 것 같아요 관련 자료 첨부할게요 🙃
| const taskViewModel = new TaskViewModel(); | ||
| const subjectViewModel = new SubjectViewModel(taskViewModel); |
There was a problem hiding this comment.
index.js 스크립트 요소를 모델과 관련된 js 파일 이후에 배치하여 새로운 객체를 이용하시는 모습이 정말 놀라워요!
Javascript에서 class는 사실 java나 C++과는 다르게 syntatic sugar에 해당한다는 것을 알고 계시나요? 이는 js가 프로토타입 기반의 프로그래밍 언어라서 그런데요, 관련 개념을 공부하시면서 프로토타입 체이닝과 같은 주제도 함께 살펴보시는 것을 추천드려요 :)
There was a problem hiding this comment.
헉 그런가요?! class가 syntatic sugar인건 몰랐는데 공부해야겠네요😋 참고 링크도 감사합니다!!
| function createElement(tag, attributes = {}) { | ||
| const element = document.createElement(tag); | ||
|
|
||
| // Iterate provided attributes | ||
| Object.entries(attributes).forEach(([key, value]) => { | ||
| if (key in element) { | ||
| // Case #1 | ||
| // If the attribute key is a property of the element, set it. | ||
| // i.e., properties like `value`, `checked`, `textContent` | ||
| element[key] = value; | ||
| } else { | ||
| // Case #2 | ||
| // If it is not, set it as an HTML attribute. | ||
| // i.e., attributes like `type`, `id`, `class` | ||
| element.setAttribute(key, value); | ||
| } | ||
| }); |
There was a problem hiding this comment.
다양한 DOM 요소를 만들기 위한 CreateElement()를 함수로 만들어주셔서, 매번 불필요하게 코드가 길어지는 것보다 맥락(context)를 제공하는 모습이 너무 좋습니다!
| } | ||
|
|
||
| #createFormElement(subjectId) { | ||
| const formElement = createElement('form', { |
There was a problem hiding this comment.
개인적으로 갑자기 CreateElement() 함수가 등장해서, "어 이건 어디에서 정의된 거지?" 라는 생각이 잠깐 스쳐 지나가긴 했었어요!
그래도 js 파일이 순서대로 로드되고 있기에 해당 메서드들을 참조할 수 있고 의미론적으로 좋은 함수 네이밍을 활용해주셔서 이해하기에 좋았어요. 다만, js 파일을 하나만 붙여주시고 나머지 js 파일들은 해당 js 파일에서 import 해주는 것은 어떨까 싶어요! 물론 브라우저 동작에 따라 보안적으로 괜찮은지...? 도 생각해볼 여지가 있다고 생각합니다
There was a problem hiding this comment.
맞아요... 그리고 vscode에서 함수 정의가 어디 있는지 못 찾는 경우도 생기더라구요. <script type="module" ...처럼 쓰면 될 것 같은데 저는 계속 오류가 나서 적용하지 못했어요😭 리뷰하면서 보니 혜인님은 import/export를 잘 쓰고 계신 것 같아서 여쭤보려고요!!
| class Subject { | ||
| #title; | ||
| #state; | ||
| #id; | ||
|
|
||
| constructor({ title = NEW_SUBJECT_NAME, state = OPEN }) { | ||
| this.#title = title; | ||
| this.#state = state; | ||
| this.#id = getRandomId(); | ||
| } | ||
|
|
||
| setState(state) { | ||
| this.#state = state; | ||
| } | ||
|
|
||
| getTitle() { | ||
| return this.#title; | ||
| } | ||
|
|
||
| getState() { | ||
| return this.#state; | ||
| } | ||
|
|
||
| getId() { | ||
| return this.#id; | ||
| } | ||
| } |
There was a problem hiding this comment.
기본 속성을 private으로 선언해두고, 이에 대한 접근과 수정을 getter와 setter로 진행하는 것은 객체지향 프로그래밍의 기본과도 같다고 할 수 있죠!
| getSubjectId() { | ||
| return this.#subjectId; | ||
| } | ||
| } |
There was a problem hiding this comment.
데이터를 다루는 비즈니스 로직을 담은 task와 subject를 모델로 분리해주셨네요! 실질적인 view로부터 이벤트를 감지하고 해당 모델에 action(?이라고 해야하나 이벤트라고 해야하나 애매하네욥 🥲)에 따른 처리는 viewModel 파일에서 진행되구요.
angularJS와 같은 기존의 SPA 프레임워크는 MVC 패턴을 가져 후에 nestJS 같은 백엔드 기술에도 영향을 주었는데, VueJS 같은 비교적 최근에 나타난 기술은 mvvm 패턴을 보이는 것으로 알고 있어요. 다혜님께서도 리드미에 이를 너무 잘 작성해주셨고요.
다만 개인적으로 react는 Vue()와 같은 인스턴스가 따로 있지는 않고 jsx 문법을 통해 컴포넌트를 만들고 virtual DOM을 통해 최적화를 시킨다는 점에 있어서는 약간의 차이가 있다고 생각해요! 나중에 다혜님이랑 이와 관련하여 많은 토론 해보면 좋겠어요 👍
There was a problem hiding this comment.
평소에 React 말고 다른 프레임워크도 잘 살펴보시나봐요 저는 React쪽만 써봤거든요😭😭😭 저도 열심히 공부해서 토론할 만큼의 지식을 키워보겠습니당ㅋㅋㅋㅋㅋ 리뷰 너무 고생하셨습니다!👍👍👍
| #addTaskEventHandler({ | ||
| task, | ||
| taskElement, | ||
| deleteButtonElement, | ||
| checkboxElement, | ||
| }) { | ||
| deleteButtonElement.addEventListener('click', () => { | ||
| this.deleteTask(task.getId(), task.getSubjectId()); | ||
| dispatchTaskChangeEvent(deleteButtonElement); | ||
| }); | ||
|
|
||
| checkboxElement.addEventListener('change', (event) => { | ||
| task.setIsCompleted(event.target.checked); | ||
| taskElement.classList.toggle('isCompleted'); | ||
|
|
||
| dispatchTaskChangeEvent(checkboxElement); | ||
| }); | ||
| } |
There was a problem hiding this comment.
사실 전체적으로 코드를 보면서 dom 이벤트와 알고리즘적인 요소를 매우 잘 조화시킨 것 같아요. 현재 객체 인스턴스 내에서만 사용할 메서드는 #을 붙여 은닉화를 해주시고 특정 문제의 해결을 위한 알고리즘에 집중했다는 점에서 응집성이 매우 높다고 생각해요.
그 중간 중간 DOM 이벤트의 알맞은 태스크를 미리 판별하는 if문도 적극 활용해주셔서 얼리 리턴문을 유용하게 사용하라는 프로그래밍 원칙과도 잘 닮아있다고 생각합니다.
| <link rel="stylesheet" href="css/reset.css" /> | ||
| <link rel="stylesheet" href="css/global.css" /> | ||
| <link rel="stylesheet" href="css/kanban.css" /> | ||
| <link rel="stylesheet" href="css/subject.css" /> | ||
| <link rel="stylesheet" href="css/task.css" /> | ||
| <link rel="stylesheet" href="css/variables.css" /> |
There was a problem hiding this comment.
style을 많이 나누어주셨네요!!!
reset.css와 같은 경우는 global.css에 @import해놓는 것도 좋을 것 같습니당
| @@ -0,0 +1,13 @@ | |||
| // kanban board columns name | |||
| @@ -0,0 +1,18 @@ | |||
| :root { | |||
There was a problem hiding this comment.
미리 theme화 시켜서 자주 쓰는 색깔을 지정해 주신게 많이 고민하신 것 같다는 생각이 들어요!
| * | ||
| * @returns {HTMLElement} - The created element with the specified attributes. | ||
| */ | ||
| function createElement(tag, attributes = {}) { |
There was a problem hiding this comment.
깔끔하게 html 생성 요소 함수를 재활용할 수 있게 작성해주신 것 같아요!
| } | ||
|
|
||
| function getRandomId() { | ||
| return Math.random() // Generate random number between 0 to 1. |
There was a problem hiding this comment.
고유성을 보장할 때에는 uuid 같이 고유성이 보장된 생성 방식을 사용하는 게 좋다고는 합니다
There was a problem hiding this comment.
헙 그냥 생각없이 random 때렸는데 그러게용..,, 고유성을 보장할 수 있는 방법을 찾아보겠습니다!
| function dispatchTaskChangeEvent(element) { | ||
| const taskChangeEvent = new CustomEvent('taskChange', { | ||
| bubbles: true, |
There was a problem hiding this comment.
여기 detail을 추가해서, 이벤트 발생할 때 넘기고 싶은 정보를 담는 것도 보다 더 재사용성을 높일 수 있을 것 같습니다!
| function dispatchTaskChangeEvent(element) { | |
| const taskChangeEvent = new CustomEvent('taskChange', { | |
| bubbles: true, | |
| function dispatchTaskChangeEvent(element, data={}) { | |
| const taskChangeEvent = new CustomEvent('taskChange', { | |
| bubbles: true, | |
| detail: data |
| this.#subjectList | ||
| .get(state) // Get the column where the subject will be inserted | ||
| .push(newSubject); |
There was a problem hiding this comment.
this.#subjectList.get(state) 가 undefined일 가능성을 처리해주는 게 좋을 것 같습니당!
There was a problem hiding this comment.
아 맞아요 예외처리...🥲 다른 부분도 꼼꼼하게 살펴보겠습니다!! 감사해요😘😘
| deleteSubject({ targetId, state }) { | ||
| // Get the column from which the subject will be removed | ||
| const subjectList = this.#subjectList.get(state); | ||
|
|
||
| // Get the index of the target subject by its ID | ||
| const targetIndex = subjectList.findIndex( | ||
| (subject) => subject.getId() === targetId | ||
| ); | ||
| // Remove the target subject from the column | ||
| subjectList.splice(targetIndex, 1); | ||
|
|
||
| this.render(); | ||
| } | ||
|
|
There was a problem hiding this comment.
| deleteSubject({ targetId, state }) { | |
| // Get the column from which the subject will be removed | |
| const subjectList = this.#subjectList.get(state); | |
| // Get the index of the target subject by its ID | |
| const targetIndex = subjectList.findIndex( | |
| (subject) => subject.getId() === targetId | |
| ); | |
| // Remove the target subject from the column | |
| subjectList.splice(targetIndex, 1); | |
| this.render(); | |
| } | |
| deleteSubject({ targetId, state }) { | |
| // Get the column from which the subject will be removed | |
| const subjectList = this.#subjectList.get(state); | |
| // Get the index of the target subject by its ID | |
| const targetIndex = subjectList.findIndex( | |
| (subject) => subject.getId() === targetId | |
| ); | |
| // Remove the target subject from the column | |
| subjectList.splice(targetIndex, 1); | |
| if(targetIndex !== -1) { | |
| subjectList.splice(targetIndex, 1); | |
| } | |
| this.render(); | |
| } | |
요렇게 예외처리를 추가해주는 것도 좋을 것 같아용
There was a problem hiding this comment.
prettier로 작성한 코드를 보기 좋게, 또 일관성 있게 포맷팅을 해주는 작업 해주신 게 좋은 거 같아요 ! 저도 다음에는 prettier 설정 파일을 사용해보겠습니다
| * @returns {Subject[]} - An array of subjects associated with the specified state. | ||
| */ | ||
| #getSubjectsByState(state) { | ||
| return this.#subjectList.get(state) || []; |
There was a problem hiding this comment.
.get(state) || []를 사용하여 undefined 반환 에러를 예방하셨네요! 배우고 갑니다 👍🏻
| <body> | ||
| <div class="container"></div> | ||
| <div class="wrapper"> | ||
| <div class="container"> | ||
| <header> | ||
| <h1 id="title" class="title"></h1> | ||
| </header> | ||
|
|
||
| <div class="kanban-board"> | ||
| <section class="board-column" id="open-column"> | ||
| <h2>Open</h2> | ||
| <hr class="open-hr" /> | ||
| <ul class="subject-list" id="open-subject-list"></ul> | ||
| </section> | ||
|
|
||
| <section class="board-column" id="in-progress-column"> | ||
| <h2>In Progress</h2> | ||
| <hr class="in-progress-hr" /> | ||
| <ul class="subject-list" id="in-progress-subject-list"></ul> | ||
| </section> | ||
|
|
||
| <section class="board-column" id="done-column"> | ||
| <h2>Done</h2> | ||
| <hr class="done-hr" /> | ||
| <ul class="subject-list" id="done-subject-list"></ul> | ||
| </section> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </body> |
결과물
배포 링크 :
https://vanilla-todo-20th-lovat.vercel.app/
예시 이미지:
기능 구현
느낀 점
저는 최근에 VanillaJs로 프로젝트를 진행한 적이 있습니다. 그때 MVVM같은 패턴을 적용하지 못해서 정말로 아쉬웠는데요, 이번에 공부하면서 흉내를 조금 내봤습니다.
결과적으로 코드가 많이 길어져서 읽기 힘들어졌는데, 대신에 최대한 주석을 열심히 달아보려고 노력했습니다🥲
새로운 목표나 할 일을 JS에서 동적으로 추가하게 되면서 DOM element를 생성하는 코드를 작성했는데, 이 부분 코드가 알아보기 너무 어려운 건 아닌지, 지나친 렌더링을 야기하는 건 아닌지 고민해 볼 시간이 부족해 아쉬웠습니다.
CSS 스타일링 역시 필요 없는 className을 많이 사용한 건 아닌지, 비효율적인 부분이 있는지 잘 모르겠네요😭
Key Questions
DOM은 무엇인가요?
DOM(Document Object Model)은 HTML 또는 XML 문서의 구조를 표현하는 인터페이스로, 프로그래밍 언어가 DOM 구조에 접근할 수 있는 방법을 제공합니다.
예를 들어서, JS라는 프로그래밍 언어로 HTML 문서의 구조, 스타일, 내용을 변경할 수 있습니다.
HTML 문서가 있다면, 브라우저는 HTML 문서를 읽어들이고 HTML의 각 요소(element)를 Node라는 객체로 표현합니다.
<h1>~<h6>태그는HTMLHeadingElement객체로,<p>는HTMLParagraphElement객체로 표현하며, 이런HTMLElement인터페이스를 통해서 HTML의 element를 수정할 수 있습니다.이벤트 흐름 제어(버블링 & 캡처링)이 무엇인가요?
focus,blur등의 일부 이벤트는 버블링 단계를 거치지 않습니다.클로저와 스코프가 무엇인가요?
참고