[뉴스 뷰어 페이지 2단계] 이수진 미션 제출합니다. by sujin0707 · Pull Request #7 · LIN-KHU/javascript-newsviewer

sujin0707 · 2024-02-03T19:13:35Z

No description provided.

tkdrb12

안녕하세요 수진님
설 연휴는 잘 보내셨나요?

이번 미션도 수행하시느라 고생많으셨습니다.
제출해주신 pr을 읽어보니 openModal과 같이 너무 길게 작성된 메서드가 보이는 것 같아요
아직 어려우시겠지만 메서드는 하나의 메서드가 하나의 역할만 수행하도록 작성해보시면 좋을 것 같아요. 처음에는 코드 라인수를 제한을 두고 진행해보는 걸 추천드립니다.

tkdrb12 · 2024-02-11T06:40:40Z

src/index.js

@@ -68,61 +68,92 @@ class App {
    const modal = document.getElementById("modal");


전체적으로 App클래스에서 담당하는 일이 너무 많아진 것 같아요
다음 미션에서는 컴포넌트와 파일을 어떻게 분리를 해야할 지를 중점적으로 코드를 작성해보면 좋을 것 같아요

tkdrb12 · 2024-02-11T06:42:30Z

index.html

+        <div class="close-modal">&times;</div>
+        <div id="modal-header" class="modal-header"></div>
        <h2>의견 남기기</h2>
        <div class="model-section">


사소하지만 model이라고 오타가 발생한 것 같아요!

오타 수정하겠습니다

tkdrb12 · 2024-02-11T06:44:29Z

index.html

@@ -16,7 +16,8 @@ <h1>LINKHU<span>-NEWS</span></h1>
    </div>
    <div id="modal" class="modal">


모달 html요소를 직접적으로 html 내에 작성하기 보다는 별도로 js파일을 만들어 작성하는걸 추천드립니다.
html내에는 <div id="root-modal"> 과 같이 진입점만 존재해도 될 것 같아요

tkdrb12 · 2024-02-11T06:51:21Z

src/index.js

    const closeModal = document.querySelector(".close-modal");
    const opinionForm = document.getElementById("opinion-form");
-    const opinionTitleInput = document.getElementById("opinion-title");
-    const opinionContentTextArea = document.getElementById("opinion-content");
+    const opinionTitle = document.getElementById("opinion-title");
+    const opinionContent = document.getElementById("opinion-content");


다음과 같이 html에 직접적으로 파일을 작성하다보니 modal의 요구사항이 변경되면 유지보수가 불편해질 수 있어요.

예를 들어 모달의 form에 태그도 작성할 수 있도록 입력창을 추가하라는 요구사항이 생겼다면 html의 내용을 수정하고 index 파일내에서 코드를 처음부터 다시 읽고 해당 단락을 찾아 내용을 추가해야 할 겁니다. 파일의 구조가 복잡해지고 코드의 작성자가 아니라면 유지 보수를 하기가 까다롭겠죠?

코드 작성시 이런 점을 유의해서 코드를 작성하면 좀 좋은 코드를 작성할 수 있을거라 생각해요

알겠습니다! 해당 부분대로 html에 작성하는 걸 지양하는 방식으로 수정하려 했는데 계속해서 오류가 생겨서 일단 이 부분은 수정하지는 못했습니다 ㅠㅠ 이후에는 이런 방식으로 작성하지 않도록 해보겠습니다!

tkdrb12 · 2024-02-11T06:52:08Z

src/index.js

+    const opinionTitle = document.getElementById("opinion-title");
+    const opinionContent = document.getElementById("opinion-content");

    modal.style.display = "block";


css관련 내용은 style파일에 보관하는 걸 추천드립니다.

css파일에 보관되도록 수정하겠습니다!

tkdrb12 · 2024-02-11T07:04:42Z

src/index.js

@@ -68,61 +68,92 @@ class App {
    const modal = document.getElementById("modal");


해당 메소드에 부여된 기능이 너무 많습니다.
openModal명의 메소드에 기대할 수 있는 기능은 이미 모달을 렌더링하는 것이잖아요?
하지만 해당 메소드는 모달에 내용을 정의하고 다양한 이벤트를 부여하고 있습니다.
다음 미션에서는 메소드의 단위를 작게 나누는 걸 추천드립니다.

(메소드 하나 당 하나의 기능을 가져도록 연습해보세요!)

네 알겠습니다! 다음 미션부터 기능별로 메소드 단위를 작게 나눠보겠습니다

tkdrb12 · 2024-02-11T07:08:10Z

src/index.js

+    const storedOpinion = localStorage.getItem(articleTitle);
+
+    if (storedOpinion) {
+      const parseOpinion = JSON.parse(storedOpinion);


해당 단락은 별도의 유틸함수로 분리할 수 있겠네요

modal.js 파일을 따로 만들고, 해당 부분 별도의 함수로 분리하겠습니다.

tkdrb12 · 2024-02-11T07:15:24Z

src/index.js

+        editButton.addEventListener("click", () => {
+          const opinion = { title: opinionTitle.value, content: opinionContent.value };
+          localStorage.setItem(articleTitle, JSON.stringify(opinion));
+          modal.style.display = "none";
+        });


해당 코드 단락에는 event.preventDefault() 메소드가 필요합니다.
이유가 무엇일까요?

해당 메소드는 이벤트 동작과 관련하여 사용되는 메소드로 브라우저의 기본 동작을 실행하지 않도록 합니다. 해당 메소드를 사용함으로써 버튼을 클릭할 때 의도치 않게 페이지가 새로고침되거나 이동하지 못하도록 방지할 수 있기 때문에 필요한 것 같습니다.

tkdrb12 · 2024-02-11T07:21:45Z

src/index.js

+
+        editButton.addEventListener("click", () => {
+          const opinion = { title: opinionTitle.value, content: opinionContent.value };
+          localStorage.setItem(articleTitle, JSON.stringify(opinion));


기사의 이름으로 키값으로 설정하는 건 부적절하다고 생각합니다.
이름으로 id를 저장하면 겹치는 이름이 발생할 수도 있잖아요?
적절한 api 응답값이 없기때문에 해당 키값으로 작성하신건가요?

그 부분은 미처 고려하지 못했습니다. 기사의 url을 키값으로 설정하는 것이 더 좋아 보여서 이렇게 변경하겠습니다!

tkdrb12 · 2024-02-11T07:24:09Z

src/index.js

+        editButton.id = "edit-opinion";
+
+        editButton.addEventListener("click", () => {
+          const opinion = { title: opinionTitle.value, content: opinionContent.value };


편집하는 기능에 기존 정보를 삭제하는 내용이 존재하지 않는 것 같아요
새로운 의견을 남겼다면 기존의 의견을 삭제해야 합니다.

추가적으로 편집 시 작성된 내용이 없으면 정보를 저장하지 않도록 하는 과정도 필요하다고 생각해요 현재 작성된 내용이 없으면 오류가 발생하는 경우가 있습니다.

피드백 주신 부분대로 수정하였습니다!

tkdrb12 · 2024-02-11T07:32:17Z

pr에는 반영되지 않았지만 tag에 a태그는 하이퍼링크에 사용되는 태그이므로 다른 태그로 바꾸시는 걸 추천드립니다.

sujin0707 added 4 commits February 4, 2024 04:00

모달창 버튼 스타일 추가

ff92039

의견 남기기 모달창 기능 구현

3c59dbb

오타 수정

34a39a1

edit 버튼 기능 수정

c69c9d1

tkdrb12 requested changes Feb 11, 2024

View reviewed changes

sujin0707 added 5 commits February 15, 2024 02:46

오타 수정

4c02086

모달 기능 별도의 파일로 분리

f561487

기능에 따라 함수 총 2개로 분리

c92f379

모달창 스타일 부분 추가

be78885

preventDefault 메소드 추가

3b7d32f

		@@ -68,61 +68,92 @@ class App {
		const modal = document.getElementById("modal");

		@@ -16,7 +16,8 @@ <h1>LINKHU<span>-NEWS</span></h1>
		</div>
		<div id="modal" class="modal">

Conversation

sujin0707 commented Feb 3, 2024

Uh oh!

tkdrb12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkdrb12 commented Feb 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants