Skip to content

modified1#1

Open
eunseobb wants to merge 1 commit into
masterfrom
develop
Open

modified1#1
eunseobb wants to merge 1 commit into
masterfrom
develop

Conversation

@eunseobb
Copy link
Copy Markdown
Owner

No description provided.

@songhaeunsong
Copy link
Copy Markdown

투두랑 버튼 정렬 질문하셨는데, 한줄에 안들어가는 이유는 css파일 .listItem에서 flex-direction 이 column이어서 그런 것 같아요.
column을 row로 바꿔보세요!
js 파일 todoItemElem.appendChild(checkElem);
todoItemElem.appendChild(todoElem); 부분도 이 두개 순서를 바꾸시면 예쁘게 정렬 될 것 같습니다.

Copy link
Copy Markdown

@NaamuKim NaamuKim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

연말에 코드치시느라 고생 너무 많으셨습니다!!
새해복 많이 받으세요!
눈송이 효과 좋네요 😊

너무 고생하셨구요!
html, css, js가 다 처음이신 걸로 알고 있는데 어떠셨나요?
너무 잘하셨습니다!
리뷰를 진행하는 이유는 리뷰를 통해 생각해볼 거리들을 많이 얻으시고 학습하면서 성장하시길 바라는 마음일 거에요!
꽤 많은 생각할 거리들을 드릴 거라 조금 놀라실 수도 있는데 부담 갖지 마시고 천천히 읽어보시면서 생각해보시면 좋을 것 같아요!

생각해볼거리들 위주로 리뷰를 해볼게요!
제 생각이 정답은 아닙니다.
그냥 생각해볼 거리들을 최대한 많이 드려볼게요!
생각해보신 다음에 질문이 있으시면 언제든 편하게 해주세요!
같이 얘기 나눠봐요😊

초반부에 커밋들이 이미 마스터 브랜치에 있어 최대한 이곳에 내용을 몰아 작성해보겠습니다!

크게 생각해볼 거리들

  • 투두가 추가될 때는 무슨 동작들이 일어나야하고 어떻게 보여줘야할까요?
  • forEach를 사용하면 어떤 게 좋을까요?
    • map과 forEach는 무슨 차이가 있을까요
    • for문 대신 forEach를 사용하는 이유는 뭘까요?
  • 사용자가 브라우저를 닫아도 정보가 남아있으려면 어떻게 해야할까요?

각각의 답을 생각해보시고 코드를 추가해보시면 도움이 되실 것 같습니다.

자잘한 생각거리들 및 팁

  • enter이벤트, 버튼이벤트 등을 하나의 eventListner로 처리할 수는 없을까요?
    • form 태그 안에 input 태그를 넣어 submit 이벤트로 처리할 수 있습니다.
  • 이 투두리스트가 엄청난 인기를 끌어 기능을 200개 이상 늘렸다고 생각할 때는 어떤 구조로 코드를 작성해야할까요?
    • 객체지향 적으로 코드를 짤 수도 있고 javascript의 함수를 매개변수로 주는 테크닉을 이용할 수 있습니다. 앞으로 전공과목 공부할 때도 도움이 되는 내용이니 학습해보셔도 좋을 것 같습니다!
  • 여러명이 동시에 작업할 때는 어떻게 작업해야할까?
    • 한 파일을 동시에 작업하면 어떤 문제가 발생할까요?

javascript

let과 const의 차이

let todos = []로 선언하신 이유가 뭘까요?
const todos = [] 로 대체할 수는 없을까요?

화살표함수

화살표함수는 왜 쓸까요?

const getLeftItems = () => {
    return todos.filter(todo=> todo.complete === false);
}
// 저라면 이렇게 작성할 것 같습니다.
const isComplete = (todo) => todo.complete
const getInProgressItems = () => todos.filter(todo => !isComplete(todo))

concat과 push의 차이

concat과 push라는 메서드 둘다 배열에 요소를 추가하는 역할을 하는 데 무엇이 다를까요??
newTodo 는 어울리는 변수명일까요?
전 setTodos에 text를 던지고

const setTodos = (content) => {
  todos.push({id:id++,complete: false, content})
}

로 축약하여 작성했을 것 같습니다!

코드 컨벤션

코드를 한 기준을 만들고 이를 지키며 작성하면 예상치 못한 오류를 잡을 수 있을 뿐 아니라 협업할 때 같은 종류의 코드를 읽을 수 있어 생산성도 높이 유지할 수 있습니다.
functionconst 를 함수 선언할 때 섞어서 쓰면 함수의 호이스팅으로 함수의 위치를 바꿔야하는 상황이 생길 수 있어요! 하나로 통일해보시면 조금더 편할거에요!
let id = 0 이부분 id를 전역으로 생성하지 않고도 고유하게 유지할 수 있는 여러 방법이 있습니다.
보통 uuid라이브러리를 많이 사용하는데 조금 더 쉽게는 new Date() 를 이용하면 현재 시간이 들어가기 때문에 겹치지 않게 사용할 수 있습니다.

정말 고생 많이하셨습니다!
과제하시면서 웹 개발의 재미를 느끼고 익숙해지셨으면 좋겠네요!
화이팅입니다!

다시 한 번 새해복 많이 받으세요!

Comment thread myTodo.js
delBtnElem.classList.add('delBtn');
delBtnElem.addEventListener('click', ()=> deleteTodo(todo.id));
delBtnElem.innerHTML='🗑';
delBtnElem.innerHTML='';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checkbox와 삭제버튼은 비슷하게 생긴 UI잖아요?
스크린샷 2023-01-01 오후 10 47 06

이를 하나의 reaction같은 div로 묶고 해당 div에 flex 속성을 넣어주면 자동으로 가로배치가 됩니다!
예시 코드를 첨부하겠습니다.

        const reactionBox = document.createElement('div');
        reactionBox.className='reactionBox'
        const checkElem = document.createElement('div');
        checkElem.classList.add('checkbox');
        checkElem.addEventListener('click', ()=>completeTodo(todo.id));
        checkElem.innerHTML='😶'

        const delBtnElem = document.createElement('button');
        delBtnElem.classList.add('delBtn');
        delBtnElem.addEventListener('click', ()=> deleteTodo(todo.id));
        delBtnElem.innerHTML='❌';

        reactionBox.appendChjild(checkElem);
        reactionBox.appendCHild(delBtnElem);
.reactionBox{
  display : flex;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 지금 전체적으로 코드가 정렬이 일관적으로 안돼있는데 혹시 vscode 사용하고 계시면
prettier 라이브러리 적용하시면 저장할 때마다 같은 형식으로 코드 정렬되게끔 설정할 수 있습니다.
참고하실 글 첨부하겠습니다.
https://record22.tistory.com/112

Comment thread myTodolist.html
<div class="todoBottom">
<div class="leftItems"> </div>
</div>
<ul class="todolist"></ul>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스네임은 어떤 부분은 camelCase고 어떤 부분은 소문자인것보다 형식을 일관성있게 만들면 className실수로 html파일을 왔다갔다 할 일이 줄어들 수 있을 것 같습니다😊

Comment thread myTodolist.html
Comment on lines +23 to +24
<div class="leftItems"> </div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

공백과 줄바꿈은 모두 컴퓨터가 읽어야하는 리소스입니다.
불필요한 공백과 줄바꿈을 자제해주세요!

물론 클린코드를 위한 줄바꿈은 찬성입니다!

전 개인적으로 html 같은 태그로 작성된 html 사이에 줄바꿈은 필요한 이유가 많지 않은 것 같습니다

Comment thread myTodo.css
height: 1.5rem;
margin: 0.5rem 0.5rem;
border-radius: 50px;
border-radius: 5px;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rem과 px를 섞어서 사용하시는 이유가 있을까요?

rem을 사용하면 어떤 부분이 좋을까요?

현재 기준이 되는 rem은 몇px일까요?

기준이 되는 rem을 10px로 바꾸면 계산하기 편하지는 않을까요?

스크린샷 2023-01-01 오후 11 02 09
모바일 화면에선 이런 화면을 보게 되는데 어떻게 하면 모바일 화면에서도 보기 좋게 스타일링할 수 있을까요?

스크린샷 2023-01-01 오후 11 03 19
이모지를 가운데 배치하기 위해서 사용할 수 있는 여러 방법이 있습니다!
그 중 하나로 박스를 flex 박스로 만들어 justify-content : center align-items : center 를 사용하실 수 있습니다.

@geunu97
Copy link
Copy Markdown

geunu97 commented Jan 2, 2023

정말 잘하셨네요! 꼭 정답은 아니기에 참고 정도만 하셨으면 좋겠습니다!

  • prettier를 사용하여 코드 스타일을 통일화하여 가독성을 좋게 할 수 있을 것 같습니다.

    • 현재 myTodo.js에서 공백, 줄바꿈의 간격이 일정하지 않습니다.
    • 큰 따옴표 또는 작은 따옴표 하나만 사용하면 좋을 것 같습니다.
  • 여기서는 굳이 백틱이 아닌, 따옴표를 사용하는게 좋을 것 같습니다. (템플릿 리터럴)

    const todoInputItems = document.querySelector(`.whattodo`);
    const todoInputItems = document.querySelector('.whattodo');
    
  • 중복된 코드를 추상화하여 조금 더 간결하게 표현할 수 있을 것 같습니다.

    const selector = (node) => document.querySelector(node);
    
    const todoInputItems = selector(".whattodo");
    const todoListItems = selector(".todolist");
    const leftItems = selector(".leftItems");
    const body = selector("body");
    
    const creator = (node, className) => {
      const newNode = document.createElement(node);
      newNode.classList.add(className);
      return newNode;
    }
    
    const todoItemElem = creator("li", "listItem");
          
    const checkElem = creator("div", "checkbox");
    checkElem.addEventListener('click', ()=>completeTodo(todo.id));
    checkElem.innerHTML='😶'
    
    const todoElem = creator("div", "todo");
    todoElem.innerText=todo.content;
    
    const delBtnElem = creator("button", "delBtn");
    delBtnElem.addEventListener('click', ()=> deleteTodo(todo.id));
    delBtnElem.innerHTML='❌';
    

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants