[뉴스 뷰어 페이지 2단계] 이수진 미션 제출합니다. #7

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

@@ -16,7 +16,8 @@ <h1>LINKHU<span>-NEWS</span></h1>
    </div>
    <div id="modal" class="modal">
      <div class="modal-content">
-        <span class="close-modal">&times;</span>
+        <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 opinionTitleInput = document.getElementById("opinion-title");
-    const opinionContentTextArea = document.getElementById("opinion-content");
+    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[뉴스 뷰어 페이지 2단계] 이수진 미션 제출합니다. #7

[뉴스 뷰어 페이지 2단계] 이수진 미션 제출합니다. #7

sujin0707 commented Feb 3, 2024

tkdrb12 left a comment

tkdrb12 Feb 11, 2024

tkdrb12 Feb 11, 2024

sujin0707 Feb 14, 2024

tkdrb12 Feb 11, 2024

tkdrb12 Feb 11, 2024

sujin0707 Feb 14, 2024

tkdrb12 Feb 11, 2024

sujin0707 Feb 14, 2024

tkdrb12 Feb 11, 2024

sujin0707 Feb 14, 2024

tkdrb12 Feb 11, 2024

sujin0707 Feb 14, 2024

tkdrb12 Feb 11, 2024

sujin0707 Feb 14, 2024

tkdrb12 Feb 11, 2024

sujin0707 Feb 14, 2024

tkdrb12 Feb 11, 2024

sujin0707 Feb 14, 2024

tkdrb12 commented Feb 11, 2024

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

[뉴스 뷰어 페이지 2단계] 이수진 미션 제출합니다. #7

Are you sure you want to change the base?

[뉴스 뷰어 페이지 2단계] 이수진 미션 제출합니다. #7

Conversation

sujin0707 commented Feb 3, 2024

tkdrb12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkdrb12 commented Feb 11, 2024