Skip to content

Initial commit#1

Open
suchwoon wants to merge 1 commit intomasterfrom
sequence
Open

Initial commit#1
suchwoon wants to merge 1 commit intomasterfrom
sequence

Conversation

@suchwoon
Copy link
Owner

@suchwoon suchwoon commented Jan 4, 2023

No description provided.

Copy link

@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.

방학에 코드 작성하시느라 너무 고생하셨습니다!
너무 잘하셨어요!
코드를 작성하는 것이 재미는 있으셨나요?
좋은 경험이셨으면 좋겠습니다!

리뷰는 여러 생각할 거리들을 만들어드리고 싶어 드리는 것입니다!!
무엇이 잘못됐다를 말씀드리려는 게 아니니 가볍게 읽어보시고 여러 생각들을 해보셨으면 좋겠습니다.

이 투두리스트가 더 커진다면?

채운님의 투두리스트가 대박이 나서 더 많은 기능이 포함되고 많은 사람들이 동시에 작업한다면 현재 구조를 유지하기는 힘들지도 모릅니다!
하나의 js 파일이 3000줄이 넘어가면 유지보수할 때마다 코드를 찾기 너무 어려울 수 있을 것 같습니다!
그렇게 될 때 어떻게 파일을 분리해야할 지, 파일명은 어떻게 정할 지 등을 고려해보시면 좋을 것 같습니다🙏

저장

현재 코드로는 사용자가 브라우저를 닫으면 모든 내용은 초기화돼 사용자는 다시 접속하면 처음부터 투두리스트를 등록해야합니다!
이를 저장하려면 어떤 방법이 있을까요?
꼭 서버와 연결해야만할까요?
브라우저 스토리지에 대한 내용을 한번 확인해보시고 저장하는 기능을 넣어보시면 좋을 것 같습니다!

사용자 경험

채운님은 어떤 어플리케이션이 편하다고 느끼셨나요? 혹은 불편하다고 느끼셨다면 어떤 점 때문에 불편하다고 느끼셨나요?
어떠한 기능을 개발할 때 이를 사용자가 제일 편하게 이용하게 하려면 무슨 화면에 어떤 이벤트로 사용자가 사용할 수 있게 해야할까요?
고민해보시면 좋을 것 같습니다!

리뷰에 대해 질문이 있으시면 편하게 남겨주시구요!
정말 수고하셨습니다!
방학 즐겁게 보내세요!

Comment on lines +13 to +17
<div class="container">
<input id="inputField" type="text">
<button id="addToDo"> + </button>
<div class="to-dos" id="toDoList"> </div>
</div>
Copy link

Choose a reason for hiding this comment

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

카멜 케이스(inputFiled, addToDo)와 케밥케이스(to-dos)를 같은 파일에서 사용하시는 것은 코드의 일관성이 깨질 수 있습니다😊
하나의 케이스로 통일하시는 게 좋을 것 같습니다.
보통 classname이나 id는 케밥케이스로 잇는 경우가 많습니다.
이를 통일하기 위해 bem 방법론이나 여러 방법론을 통해 className이나 id 이름을 지을 때 기준을 정해서 협업을 하곤 합니다.
링크 첨부할테니 한번 편하게 읽어보시면 좋을 것 같습니다,
https://whales.tistory.com/33

<button id="addToDo"> + </button>
<div class="to-dos" id="toDoList"> </div>
</div>
<script src="main.js"> </script>
Copy link

Choose a reason for hiding this comment

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

script는 body에 맨 마지막에만 삽입할 수 있을까요?
맨 마지막에 삽입하면 어떤 점이 좋을까요?
이 부분은 관심이 생기면 한번 찾아보시면 브라우저가 html을 읽어 보여주는 과정이 이해가 되실 거에요

Comment on lines +7 to +9
if (!inputBox.value)
alert('내용을 입력하세요!');
else
Copy link

Choose a reason for hiding this comment

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

만약 조건들이 엄청나게 많아지면 어떻게 될까요?
else if가 엄청 많아지겠죠?
이는 유지보수하기 쉽지 않습니다.
조건을 추가할 때마다 모든 조건의 종속 관계를 체크해야합니다.
이를 방지하기 위해 얼리 리턴을 하는 방식을 많이 사용하곤 합니다.
위 코드에서는


if(!inputBox.value) return alert('내용을 입력하세요!);


로 사용하면 아래 else는 필요없어지겠네요!

Comment on lines +19 to +21
list.addEventListener('dblclick', function(){
toDoList.removeChild(list);
})
Copy link

Choose a reason for hiding this comment

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

사용자는 더블클릭으로 투두리스트가 삭제된다는 것을 어떻게 알 수 있을까요?
이는 좋은 사용자 경험일까요?

Comment on lines +16 to +18
list.addEventListener('click', function(){
list.style.textDecoration = "line-through";
})
Copy link

Choose a reason for hiding this comment

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

만약 이미 line-through인 list는 line-through를 지워주고 line-through가 없는 리스트는 line-through를 넣어주려면 어떻게 해야할까요?

Comment on lines +7 to +9
.container {
width: 360px;
}
Copy link

Choose a reason for hiding this comment

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

어떤 기기에서나 보기 좋은 width를 설정하려면 어떻게 해야할 지 고민해보셔도 좋을 것 같아요!
여러 경우의 수를 꼼꼼하게 고민해보시면 더 많은 내용을 깊게 생각해보실 수 있을거에요!
반응형 스타일링에 대한 글을 첨부할테니 시간 나실 때 한번 읽어보셔요😊
https://www.daleseo.com/css-responsive-layouts/

Comment on lines +20 to +22
.to-dos {
margin-top:25px;
} No newline at end of file
Copy link

Choose a reason for hiding this comment

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

이 부분은 띄어쓰기가 위에 내용과 다르네요!
매번 띄어쓰기등을 생각하며 개발하는 것은 힘든 일이라고 생각합니다!

이러한 코드들의 정렬을 도와주는 prettier라는 라이브러리가 있습니다.
설정하시면 도움되실 것 같아 설정하는 방법의 링크를 첨부하겠습니다!
https://record22.tistory.com/112

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.

2 participants