Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[뉴스 뷰어 페이지 2단계] 곽가영 미션 제출합니다. #8

Open
wants to merge 2 commits into
base: gykwak03
Choose a base branch
from

Conversation

gykwak03
Copy link

@gykwak03 gykwak03 commented Feb 3, 2024

안녕하세요.

1단계까진 비슷하게 한 것 같은데
2단계에서 모달에 하나의 의견만 띄우는 것이 아니라 여러 개의 의견을 띄우는 줄 알고 구현하다보니
수정기능을 구현하기가 애매해졌습니다.
로컬 스토리지와 같은 큰 기능은 모두 구현하였습니다.

감사합니다.

Copy link
Contributor

@tkdrb12 tkdrb12 left a comment

Choose a reason for hiding this comment

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

안녕하세요 가영님
여행은 잘 다녀오셨나요?

이번 미션도 작성하시느라 고생많으셨습니다.
아래 코멘트에는 적지 않았지만 현재 App클래스가 하는일이 너무 많아보여요
Modal 관련기능을 App클래스에서 분리하며 코드를 개선해볼 수 있을 것 같습니다.
또한 index.js에 전체 코드 내용을 두기보다는 클래스와 유틸 함수를 별도의 파일로 분리해보는 걸 추천드립니다.

Comment on lines 167 to 183
position: absolute;
width: 400px;
height: auto;
left: 508px;
top: 226px;
left: 150px; /* 수정된 값 */
top: 250px; /* 수정된 값 */
background: #FFFFFF;
border-radius: 20px;
padding: 20px;
box-sizing: border-box;
border-width: 100px;
border-color: #000000;
}


.comment2{
/* Rectangle 45 */

Copy link
Contributor

Choose a reason for hiding this comment

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

최소한의 css가 되어있지 않으면 사용자가 어플리케이션을 사용할 때 불편함을 느낄 수 있어요

  1. 버튼에 기본 스타일 초기화가 되어 있지 않아요.
  2. 댓글 입력 시 댓글을 많이 입력하면 모달밖으로 댓글이 빠져나옵니다.
  3. 모달을 닫기 위한 버튼의 위치가 헷갈려요 보통 닫기 버튼은 좌우 측 상단에 위치합니다. (모달의 backdrop을 클릭하거나 esc키를 눌렀을 때 모달이 닫아지는 기능을 고려해볼 수 있겠죠?)
  4. 모달의 위치가 고정되어 있습니다. 하단의 게시물을 클릭했을 때 모달창을 볼 수가 없어요

Comment on lines +1 to +32
class BankAccount {
constructor() {
// 밑줄로 시작하는 네이밍 컨벤션을 통해 private 속성임을 나타냄
this._balance = 0;
}

// setter 메서드를 통해 _balance 속성에 접근
set balance(newBalance) {
if (newBalance >= 0) {
this._balance = newBalance;
} else {
console.log("잘못된 금액입니다.");
}
}

// getter 메서드를 통해 _balance 속성을 읽음
get balance() {
return this._balance;
}
}

const account = new BankAccount();

// setter를 통한 값 변경
account.balance = 1000;

// getter를 통한 값 읽기
console.log(account.balance); // 1000

// 잘못된 값으로 setter 호출
account.balance = -500; // "잘못된 금액입니다." 출력
console.log(account.balance); // 1000 (이전 값 유지)
Copy link
Contributor

Choose a reason for hiding this comment

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

발표를 위한 코드 단락인가요?

Copy link
Author

Choose a reason for hiding this comment

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

앗 네네 잘못 들어갔네요 삭제하겠습니다.

Comment on lines +170 to +171
left: 150px; /* 수정된 값 */
top: 250px; /* 수정된 값 */
Copy link
Contributor

Choose a reason for hiding this comment

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

따로 주석에 입력하지 않아도 pr요청에서 볼 수 있어요👍

image

Copy link
Author

Choose a reason for hiding this comment

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

삭제하겠습니다!!

}

createCommentSection(sectionElements) {
sectionElements.forEach((section) => {
section.addEventListener("click", () => {
console.log(section);
Copy link
Contributor

Choose a reason for hiding this comment

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

console.log는 삭제해주세요!

Copy link
Author

Choose a reason for hiding this comment

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

넵!!

Comment on lines +256 to +259
deleteButton.addEventListener("click", () => {
this.deleteComment(commentElement, deleteButton);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

필요 시 event.preventDefault함수를 사용해야 합니다.
이 함수는 어떤 함수일까요?

Copy link
Author

Choose a reason for hiding this comment

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

기능 구현 중에 댓글이 쓰여지는 지 테스트 하다 보니까 댓글이 쌓이게 되어
쓰여진 댓글들을 모두 지우는 기능을 넣고 싶었습니다.

그런데 이걸 사용자가 접근할 수 있게끔 하는건 아닌 것 같아서 UI상에는 나타내지 않았습니다.

Comment on lines +263 to +266
deleteComment(commentElement, deleteButton) {
commentElement.remove();
deleteButton.remove();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

현재 이 메소드에 localstorage의 내용을 삭제하는 부분이 없어요
모달창을 닫고 다시 열면 해당 내용이 그대로 남아있어요

혹시 해당 키값을 삭제하고 싶었다면 filter 메소드 사용을 권장드립니다.
reduce, map같이 배열 메소드를 학습해보시는 걸 추천드려요

Comment on lines +269 to +270
const storedComments = JSON.parse(localStorage.getItem("comments")) || [];
this.countPeople = storedComments.length;
Copy link
Contributor

Choose a reason for hiding this comment

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

localstorage정보를 가져오는 기능을 간결하게 작성해주셨네요
해당 함수를 별도의 유틸함수로 분리해보시는 걸 추천드립니다.

Comment on lines +283 to +286
storedComments.push(commentContent);
// 배열을 다시 로컬 스토리지에 저장
localStorage.setItem("comments", JSON.stringify(storedComments));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

현재는 모든 기사가 같은 댓글창을 공유하고 있어요
기사별로 댓글을 작성할 수 있도록 해보는 걸 추천드립니다.

});
});
}

loadExistingComments(commentSection2) {
const storedComments = JSON.parse(localStorage.getItem("comments"));
if (storedComments && storedComments.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

코멘트 배열과 해당 배열의 길이를 둘다 검사한 이유가 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

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

앗 두 방법으로 할 수 있다는걸 메모해두다가 저렇게 된 것 같습니다.
하나만 사용하겠습니다!

localStorage.setItem("comments", JSON.stringify(storedComments));
}

resetLocalStorage(commentSection2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

resetLocalStorage에서 사용되는 전체 댓글 삭제는 현재 사용되지 않은 것 같습니다. 기능을 제외한 이유가 있으신가요?

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