[1단계 - todo list] 검프(김태정) 미션 제출합니다 #43
Conversation
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <module type="JAVA_MODULE" version="4"> | ||
| <component name="NewModuleRootManager" inherit-compiler-output="true"> | ||
| <exclude-output /> | ||
| <content url="file://$MODULE_DIR$" /> | ||
| <orderEntry type="inheritedJdk" /> | ||
| <orderEntry type="sourceFolder" forTests="false" /> | ||
| </component> | ||
| </module> |
There was a problem hiding this comment.
IDE 때문에 생기는 파일인거 같은데 .gitignore에 추가하면 어떨까요?!
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project version="4"> | ||
| <component name="JavaScriptLibraryMappings"> | ||
| <includedPredefinedLibrary name="Node.js Core" /> | ||
| </component> | ||
| </project> |
There was a problem hiding this comment.
IDE 때문에 생기는 파일인거 같은데 .gitignore에 추가하면 어떨까요?!
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project version="4"> | ||
| <component name="ProjectModuleManager"> | ||
| <modules> | ||
| <module fileurl="file://$PROJECT_DIR$/.idea/js-todo-list-step1.iml" filepath="$PROJECT_DIR$/.idea/js-todo-list-step1.iml" /> | ||
| </modules> | ||
| </component> | ||
| </project> |
There was a problem hiding this comment.
IDE 때문에 생기는 파일인거 같은데 .gitignore에 추가하면 어떨까요?!
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project version="4"> | ||
| <component name="VcsDirectoryMappings"> | ||
| <mapping directory="" vcs="Git" /> | ||
| </component> | ||
| </project> |
There was a problem hiding this comment.
IDE 때문에 생기는 파일인거 같은데 .gitignore에 추가하면 어떨까요?!
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <project version="4"> | ||
| <component name="ChangeListManager"> | ||
| <list default="true" id="4227aec4-9c7c-4564-8750-8d59b0dfd71b" name="Default Changelist" comment=""> | ||
| <change afterPath="$PROJECT_DIR$/src/js/app2.js" afterDir="false" /> | ||
| <change beforePath="$PROJECT_DIR$/.idea/workspace.xml" beforeDir="false" afterPath="$PROJECT_DIR$/.idea/workspace.xml" afterDir="false" /> | ||
| <change beforePath="$PROJECT_DIR$/index.html" beforeDir="false" afterPath="$PROJECT_DIR$/index.html" afterDir="false" /> | ||
| <change beforePath="$PROJECT_DIR$/src/js/app.js" beforeDir="false" afterPath="$PROJECT_DIR$/src/js/app.js" afterDir="false" /> | ||
| </list> | ||
| <option name="SHOW_DIALOG" value="false" /> | ||
| <option name="HIGHLIGHT_CONFLICTS" value="true" /> | ||
| <option name="HIGHLIGHT_NON_ACTIVE_CHANGELIST" value="false" /> | ||
| <option name="LAST_RESOLUTION" value="IGNORE" /> | ||
| </component> | ||
| <component name="FileTemplateManagerImpl"> | ||
| <option name="RECENT_TEMPLATES"> | ||
| <list> | ||
| <option value="HTML File" /> | ||
| <option value="JavaScript File" /> | ||
| </list> | ||
| </option> | ||
| </component> | ||
| <component name="Git.Settings"> | ||
| <option name="RECENT_GIT_ROOT_PATH" value="$PROJECT_DIR$" /> | ||
| </component> | ||
| <component name="ProjectId" id="1q9kc5EfZmtIZUJ69jKNa02mdAh" /> | ||
| <component name="ProjectLevelVcsManager"> | ||
| <ConfirmationsSetting value="2" id="Add" /> | ||
| </component> | ||
| <component name="ProjectViewState"> | ||
| <option name="hideEmptyMiddlePackages" value="true" /> | ||
| <option name="showLibraryContents" value="true" /> | ||
| </component> | ||
| <component name="PropertiesComponent"> | ||
| <property name="ASKED_SHARE_PROJECT_CONFIGURATION_FILES" value="true" /> | ||
| <property name="DefaultHtmlFileTemplate" value="HTML File" /> | ||
| <property name="RunOnceActivity.OpenProjectViewOnStart" value="true" /> | ||
| <property name="RunOnceActivity.ShowReadmeOnStart" value="true" /> | ||
| <property name="WebServerToolWindowFactoryState" value="false" /> | ||
| <property name="aspect.path.notification.shown" value="true" /> | ||
| <property name="javascript.nodejs.core.library.configured.version" value="14.15.0" /> | ||
| <property name="javascript.nodejs.core.library.typings.version" value="14.14.35" /> | ||
| <property name="js.last.introduce.type" value="CONST" /> | ||
| <property name="last_opened_file_path" value="$PROJECT_DIR$" /> | ||
| <property name="nodejs_interpreter_path" value="$APPLICATION_CONFIG_DIR$/node/node-v14.15.0-darwin-x64/bin/node" /> | ||
| <property name="nodejs_package_manager_path" value="npm" /> | ||
| <property name="settings.editor.selected.configurable" value="preferences.lookFeel" /> | ||
| </component> | ||
| <component name="RunManager"> | ||
| <configuration default="true" type="ArquillianTestNG" factoryName="" nameIsGenerated="true"> | ||
| <option name="arquillianRunConfiguration"> | ||
| <value> | ||
| <option name="containerStateName" value="" /> | ||
| </value> | ||
| </option> | ||
| <option name="TEST_OBJECT" value="CLASS" /> | ||
| <properties /> | ||
| <listeners /> | ||
| <method v="2"> | ||
| <option name="Make" enabled="true" /> | ||
| </method> | ||
| </configuration> | ||
| </component> | ||
| <component name="SpellCheckerSettings" RuntimeDictionaries="0" Folders="0" CustomDictionaries="0" DefaultDictionary="application-level" UseSingleDictionary="true" transferred="true" /> | ||
| <component name="TaskManager"> | ||
| <task active="true" id="Default" summary="Default task"> | ||
| <changelist id="4227aec4-9c7c-4564-8750-8d59b0dfd71b" name="Default Changelist" comment="" /> | ||
| <created>1616499269649</created> | ||
| <option name="number" value="Default" /> | ||
| <option name="presentableId" value="Default" /> | ||
| <updated>1616499269649</updated> | ||
| <workItem from="1616499275104" duration="164000" /> | ||
| <workItem from="1616500617279" duration="10446000" /> | ||
| <workItem from="1616637885652" duration="22430000" /> | ||
| </task> | ||
| <servers /> | ||
| </component> | ||
| <component name="TypeScriptGeneratedFilesManager"> | ||
| <option name="version" value="3" /> | ||
| </component> | ||
| <component name="Vcs.Log.Tabs.Properties"> | ||
| <option name="TAB_STATES"> | ||
| <map> | ||
| <entry key="MAIN"> | ||
| <value> | ||
| <State /> | ||
| </value> | ||
| </entry> | ||
| </map> | ||
| </option> | ||
| <option name="oldMeFiltersMigrated" value="true" /> | ||
| </component> | ||
| </project> |
There was a problem hiding this comment.
IDE 때문에 생기는 파일인거 같은데 .gitignore에 추가하면 어떨까요?!
|
|
||
| function count() { | ||
| $todoCount.innerHTML = $toggleInput.children.length.toString(); | ||
| } |
There was a problem hiding this comment.
<리뷰 포인트>
- 깃허브에서 - 표시는 왜 생기는 걸까?
- End of Line 은 무엇일까?
- End of Line 이 파일 마지막에 자동으로 추가되게끔 IDE에서 설정할 수는 없을까?
https://minz.dev/19
https://thoughtbot.com/blog/no-newline-at-end-of-file
| @@ -34,5 +34,6 @@ <h1>TODOS</h1> | |||
| </div> | |||
| </main> | |||
| </div> | |||
| <script type="text/javascript" src="src/js/app.js"></script> | |||
There was a problem hiding this comment.
[심화] : 심화라는 prefix가 붙은 코멘트는 JS 또는 웹브라우져에 대한 깊은 이해가 필요해서, 필요에 따라 스킵하셔도 무방합니다~
| <script type="text/javascript" src="src/js/app.js"></script> | |
| <script type="module" src="src/js/app.js"></script> |
module을 적용하면
- HTML parsing과 비동기적으로 script를 실행하므로, 더욱 빠른 script실행이 가능해집니다. 따라서 head 태그내에 기재하는 것이 더욱 좋습니다.
- HTTP Server를 통해서만 로드됩니다. 따라서 http server 패키지를 설치하거나 live-server 익스텐션을 설치하여 개발하여야 합니다.
| { | ||
| "name": "js-todo-list-step1", | ||
| "version": "1.0.0", | ||
| "description": "<p align=\"middle\" > <img width=\"200px;\" src=\"./src/images/check_list.png\"/> </p> <h2 align=\"middle\">JS 투두리스트 스텝1</h2> <p align=\"middle\">자바스크립트로 구현 하는 투두리스트</p> <p align=\"middle\"> <img src=\"https://img.shields.io/badge/version-1.0.0-blue?style=flat-square\" alt=\"template version\"/> <img src=\"https://img.shields.io/badge/language-html-red.svg?style=flat-square\"/> <img src=\"https://img.shields.io/badge/language-css-blue.svg?style=flat-square\"/> <img src=\"https://img.shields.io/badge/language-js-yellow.svg?style=flat-square\"/> <a href=\"https://github.com/daybrush/moveable/blob/master/LICENSE\" target=\"_blank\"> <img src=\"https://img.shields.io/github/license/woowacourse/javascript-lotto.svg?style=flat-square&label=license&color=08CE5D\"/> </a> </p>", |
There was a problem hiding this comment.
description 은 npm search 커맨드를 이용해 패키지를 검색할 때 쓰여지므로, html 이 아닌 단순 string으로 작성하는 것이 좋습니다.
https://docs.npmjs.com/cli/v7/configuring-npm/package-json#description
| @@ -0,0 +1,20 @@ | |||
| { | |||
| "name": "js-todo-list-step1", | |||
| "version": "1.0.0", | |||
There was a problem hiding this comment.
아직 major 버젼이 아니므로, "0.0.1" 등 으로 하면 어떨까요?
"x.y.z"
x: 메이져 버젼
y: 마이너 버젼
z: 패치 버젼
| "url": "git+https://github.com/Livenow14/js-todo-list-step1.git" | ||
| }, | ||
| "keywords": [], | ||
| "author": "", |
There was a problem hiding this comment.
열심히 만들었는데 누가 만들었는지 author를 기재해야죠~~ 😄
| "author": "", | |
| "author": "Livenow14", |
| }, | ||
| "keywords": [], | ||
| "author": "", | ||
| "license": "ISC", |
There was a problem hiding this comment.
| "license": "ISC", | |
| "license": "MIT", |
원본 저장소의 license와 맞추는게 좋을 것 같아요
| "bugs": { | ||
| "url": "https://github.com/Livenow14/js-todo-list-step1/issues" | ||
| }, | ||
| "homepage": "https://github.com/Livenow14/js-todo-list-step1#readme" |
There was a problem hiding this comment.
별도의 설정을 하지 않았다면, remote 저장소의 gh-pages 브랜치에 푸쉬를 하시면 자동으로 github pages가 생성됩니다.
프론트엔드 개발은 왠만하면 다 github pages를 생성해서 결과물을 보여주는게 좋아요!
(Livenow14) git push origin Livenow14:gh-pages주소는 https://Livenow14.github.io/js-todo-list-step1 (https://github아이디.github.io/repository이름) 로 될꺼구요.
gh-pages 브랜치에 푸쉬 후 아래 예시 그림의 오른쪽 밑 Environments 과 같이 github pages 가 생겼는지 확인해보세요~!
|
|
||
| function addTodoItem(event) { | ||
| const todoTitle = event.target.value; | ||
| const todoList = document.getElementById("todo-list"); |
There was a problem hiding this comment.
| const todoList = document.getElementById("todo-list"); | |
| const todoList = document.querySelector("#todo-list"); |
유연성이 더 높은 querySelector 사용을 더욱 권장드립니다! 또한, 위에서도 querySelector를 사용하고 있으므로 일관되게 querySelector를 사용하시는게 좋을것 같아요!
|
|
||
| function addTodoItem(event) { | ||
| const todoTitle = event.target.value; | ||
| const todoList = document.getElementById("todo-list"); |
There was a problem hiding this comment.
| const todoList = document.getElementById("todo-list"); | |
| const $todoList = document.getElementById("todo-list"); |
HTMLElement 의 변수명에는 $를 붙이는게 암묵적인(?) 컨벤션인데, 이왕 적용하시기로 하셨다면,
$todoInput, $toggleInput 과 같이 일관성있게 다 붙이시는게 좋을 것 같아요!
| const target = event.target; | ||
| if ("toggle" === target.className) { | ||
| if ("completed" === target.closest("li").className) { | ||
| target.closest("li").className = "active"; | ||
| } else { | ||
| target.closest("li").className = "completed"; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
DOM 에 접근하는 것이 비용이 큰 작업이기 때문에, JS에서는 성능을 생각할때, 코드의 시간복잡도를 고려하는것 뿐만 아니라 DOM 조작의 최소화도 고려하여야 합니다.
target.closest("li") 가 반복적으로 호출되고 있는데요,
const $li = target.closest("li"); 과 같이 한번만 DOM 조작을 하여 변수로 할당한 후 사용하시면 좋을 것 같아요
|
|
||
| function onToggleTodoItem(event) { | ||
| const target = event.target; | ||
| if ("toggle" === target.className) { |
There was a problem hiding this comment.
| if ("toggle" === target.className) { | |
| if ("toggle" !== target.className) { | |
| return; | |
| } |
if 문 내에 if 문을 중첩시키는 것 보다 early return 하는 것이 가독성이 더 좋을것 같아요!
| if ("Escape" === event.key) { | ||
| target.closest("li").classList.remove("editing"); | ||
| } | ||
|
|
||
| if ("Enter" === event.key) { | ||
| target.innerText = editInput.value; | ||
| target.closest("li").classList.remove("editing"); | ||
| } |
There was a problem hiding this comment.
| if ("Escape" === event.key) { | |
| target.closest("li").classList.remove("editing"); | |
| } | |
| if ("Enter" === event.key) { | |
| target.innerText = editInput.value; | |
| target.closest("li").classList.remove("editing"); | |
| } | |
| if ("Escape" === event.key) { | |
| target.closest("li").classList.remove("editing"); | |
| return; | |
| } | |
| if ("Enter" === event.key) { | |
| target.innerText = editInput.value; | |
| target.closest("li").classList.remove("editing"); | |
| return; | |
| } |
"Escape" 와 "Enter"가 동시에 입력되는 경우는 없겠지만, 명시적으로 return을 기재해주면 가독성이 더 좋을 것 같아요!
| const todoList = $toggleInput.children; | ||
| for (let i = 0; i < todoList.length; i++) { | ||
| if (todoList[i].classList.contains("completed")) { | ||
| todoList[i].style.display = "none"; |
There was a problem hiding this comment.
style attribute는 CSS로만 변경하시는게 좋아요!
style은 CSS, 로직은 JS 이렇게 분리하는게 유지보수 측면에서 더 용이합니다!
| todoList[i].style.display = "none"; | |
| todoList[i].classList.add("d-none"); |
/* index.css */
.d-none {
display: none;
}| todoList[i].style.display = "none"; | ||
| } else { | ||
| countItems++; | ||
| todoList[i].style.display = "block"; |
There was a problem hiding this comment.
| todoList[i].style.display = "block"; | |
| todoList[i].classList.remove("d-none"); |
| } | ||
|
|
||
| function allTodoItem(event) { | ||
| if (event.target.className === 'all selected') { |
There was a problem hiding this comment.
만약에 event.target의 class attribute 가 "selected all" 이면 조건문이 false가 되어서 조건문 내부가 실행되지 않을텐데, 이거는 의도하신 바가 아니시겠죠?
| if (event.target.className === 'all selected') { | |
| if (event.target.classList.contains("all") && event.target.classList.contains("selected")) { |
또한, JS에서는 string 리터럴을 만들때 '(Single Quote), "(Double Quote), `(backtick) 을 모두 사용가능한데요,
하나로 일관성있게 작성하는 것이 좋습니다!
[심화] 사실 이러한 스타일링을 일관되게 해주는 prettier 라는 툴을 사용하시면, 신경안쓰셔도 되는 부분이긴 합니다 ㅎㅎ
https://prettier.io/
| for (let i = 0; i < childrens.length; i++) { | ||
| childrens[i].style.display = "block"; | ||
| } | ||
| $todoCount.innerHTML = $toggleInput.children.length.toString(); |
There was a problem hiding this comment.
JS에서는 자동형변환이 일어나므로, 반드시 String으로 변환시키지 않으셔도 됩니다!
(사실 저는 이걸 보기전에 toString 을 사용하여 String으로 변환할 생각 자체를 못해봤네요;; 너무나 동적 타입에 익숙해져버려서...)
| $todoCount.innerHTML = $toggleInput.children.length.toString(); | |
| $todoCount.innerHTML = $toggleInput.children.length; |
There was a problem hiding this comment.
생각보다 너무 잘 짜주셨네요! 저보고 갑자기 Java로 체스만들라고 하면 절대 이정도 퀄리티로 못 만들거에요...
데모페이지가 있었다면 좀 더 어떻게 실행되는지를 볼 수 있어서 좋았을 것 같은데,
사실 저의 귀차니즘으로 코드만 보고 코멘트 드린 점도 있습니다... 😢
event delegation, 함수의 선언 순서, HTMLElement 생성 등에 대한 코멘트도 드리고 싶었으나,
제가 너무 오지랖 넘치게 이것저것 말씀드린 것 같아 일단 마음 속에 넣어두었습니다...
제가 드린 코멘트는 그냥 참고 정도로 하셔서, 그냥 읽어만 봐도 좋고, 나중에 여유되실때 반영해보셔도 좋을 것 같아요! 추가질문 및 추가 리뷰요청(?)도 언제나 환영입니다~
아참 그리고, JS 문법 등에 관해서 검색하실 때 주의사항을 몇가지 말씀드리고 싶은데요
-
최근 1년내 자료만 참고한다. 웹 프론트엔드 생태계는 매우 급변합니다. 따라서 1년이 지난 자료는 현재 시점에서는 유효하지 않은 해답이거나, 오래된 문법으로 작성될 가능성이 있습니다. 브라우저는 자주 업데이트되고, JS 문법도 매년 새로운 것들이 추가됩니다. 다행인 것은, JS는 Backwards Compatibility 가 있어서, 현재 유효하게 작동된다면 추후에도 유효하게 작동되는 것을 보장합니다!
-
검색시에 mdn 을 붙여서 검색한다. mdn은 Mozilla Developer Network 의 줄임말으로, 프론트엔드개발과 관련하여 공식 Reference라고 생각하시면 됩니다. (완전 공식문서는 html5 spec, ECMAScript 등이 있습니다만, 넘무 어려워요 ㅠㅠ) 블로그 등으로 검색한 내용을 참고하여 mdn에서 정확한 내용을 확인하시는게 좋습니다.
-
mdn이 너무 어려우시면 https://javascript.info/ 을 참고하세요! 리액트 공식홈페이지에서도 JS 공부자료로 추천하는 매우 양질의 자료입니다. 한국어 번역도 잘 되어 있습니다! https://ko.javascript.info/
수고하셨습니다 👏 👏 👏 👏 👏
처음 해보는 js라 그런지, 많이 해맸고, 오류도 많은 거 같아요.
우선 기능요구 사항을 전부 구현하자! 라는 생각으로 코드를 작성했습니다.
작성하며 많은 크루들의 도움을 받았습니다. 😊
js공부를 더 열심히해서 저도 도움이 될 수 있게 노력하겠습니다.
감사합니다!