Conversation
tkdrb12
left a comment
tkdrb12
left a comment
There was a problem hiding this comment.
안녕하세요 윤서님
첫 코드리뷰를 맡게되네요!
전체적으로 깔끔하다고 느낌이 드는 코드였습니다.
css도 신경을 많이 써주신 것 같고 특히 컴포넌트 별로 파일분리를 해주신 부분이 아주 좋았습니다 👍
| @@ -0,0 +1,48 @@ | |||
| import Modal from "./Modal"; | |||
|
|
|||
| class ExistingModal extends Modal { | |||
There was a problem hiding this comment.
고민을 많이한 흔적이 보이는 코드였습니다👍
스토리지의 키값을 외부에서 받아와 접근할 수 있게한 점도 좋았고요
코드의 재사용성을 좀 더 개선해보고 싶다면 Modal 컴포넌트는 아래 사진과 같이 내용이 비어있도록 하고 외부에서 contents 또는 element를 주입할 수 있도록 개선해볼 수 있을 것 같아요
여러 페이지에서도 반복적으로 재사용할 수 있는 컴포넌트를 공통 컴포넌트라 합니다. Modal은 댓글 작성 뿐만 아니라 여러 곳에서 재사용이 될 수 있는 공통 컴포넌트로 사용할 수 있습니다. 만약 모달의 하위 컴포넌트를 외부에서 넘겨 받도록 설계할 경우 좀 더 재사용이 편해지겠죠?
|
|
||
| nav li:not(:last-child) { | ||
| margin-right: 13px; /* 마지막 요소 이외의 모든 요소에 오른쪽 여백 추가 */ | ||
| margin-right: 13px; |
There was a problem hiding this comment.
figma 시안을 따르는 게 가장 좋지만 만약 시안을 따르지 않고 ui를 설계해야 한다면 margin 값이나 아래 사진과 같이 통일하는 게 좋습니다.
| section > div > p > strong::after { | ||
| content: ' | '; /* 각 strong 태그 뒤에 구분을 위한 구분자 추가 */ | ||
| content: ' | '; | ||
| } |
There was a problem hiding this comment.
after 가상요소를 통해 | 문자를 넣어주셨군요 👍
하지만 Author, Date를 넣을 필요가 없을 것 같은데 넣어주신 이유가 있을까요?
ex) Author | 박정선
| position: relative; | ||
| } | ||
|
|
||
| nav li:not(:last-child) { |
There was a problem hiding this comment.
여러 css 선택자를 사용해보려 했다는 느낌이 들었습니다👍👍
| } | ||
|
|
||
| nav li { | ||
| /*margin-right: 10px; 이러면 마지막 항목도 띄어지니까.*/ |
There was a problem hiding this comment.
잘 구현해 주셨지만 flex의 gap 속성을 사용하면 좀 더 간편하게 해당 내용을 구현할 수 있을 것 같습니다!
| } | ||
|
|
||
| async fetchData(selectedTap) { | ||
| async fetchData(selectedTap) { |
| if (selectedTap && selectedTap !== "all") { | ||
| apiUrl = `http://newsapi.org/v2/top-headlines?country=kr&category=${selectedTap}&apiKey=${this.apiKey}`; | ||
| } else { | ||
| } else { //selectedTap이 정의되지 않은 상태. undefined. | ||
| apiUrl = `http://newsapi.org/v2/top-headlines?country=kr&apiKey=${this.apiKey}`; | ||
| } | ||
|
|
||
| const response = await fetch(apiUrl); | ||
| const response = await fetch(apiUrl); //promise객체 반환 fetch니까. |
There was a problem hiding this comment.
해당 단락과 fetch 부분은 따로 분리해볼 수 있을 것 같네요
There was a problem hiding this comment.
promise 메소드도 한번 사용해보는 것을 추천드립니다.
| closeModal() { | ||
| const sectionElement = document.querySelector("section"); | ||
| sectionElement.removeChild(this.modalElement); | ||
| this.modalElement.style.display = "none"; | ||
| this.titleInput.value = ""; | ||
| this.contentInput.value = ""; | ||
| } |
There was a problem hiding this comment.
해당 메소드를 재사용해 모달의 backdrop을 클릭했을 때나 esc키를 눌렀을 때 모달이 닫히는 기능도 고려해볼 수 있겠네요!
|
|
||
| nav li { | ||
| /*margin-right: 10px; 이러면 마지막 항목도 띄어지니까.*/ | ||
| cursor: pointer; |
| @@ -0,0 +1,69 @@ | |||
| class Modal { | |||
There was a problem hiding this comment.
다른 클래스에서는 element 반환하는 getElement메소드가 있었는데 Modal에는 사용하지 않은 이유가 있나요?
추가적으로 질문드리면 모달에는 쓰기 기능이 있잖아요? 그렇다면 모달 컴포넌트에서 rest api를 사용할 때 fetch의 인자는 어떤값이 들어갈 수 있을까요?
2 participants
No description provided.