Kdt5 kim gyeong won 6__재업 #66
Conversation
iamidlek
left a comment
iamidlek
left a comment
There was a problem hiding this comment.
고생하셨습니다.
전체적으로 잘 구현해 주셨습니다.
README도 잘 작성해 주셨습니다.
라우터 등 여러모로 고민하신 부분이 있어 보여 좋은 것 같습니다.
commit도 나누려고 노력하신 모습이 보입니다.
commit 의 경우 메세지를 조금 더 신경써서 어떤 작업인지 알 수 있으면 좋을 것 같습니다.
src 밖에 scss나 main.js가 있는 부분은 일반적이진 않은 것 같습니다. src 안에서 관리해 주세요.
함수형으로 구현하셨는데 거의 class로 구현하신 것 같은 느낌이 들었습니다.
store와 같이 값을 가지고 있는 객체가 있어도 좋을 것 같습니다.
src/utils/querySelector.js
Outdated
| @@ -0,0 +1,6 @@ | |||
| export const $ = (selector) => { | |||
| const result = document.querySelector(selector); | |||
| if (!(result instanceof HTMLElement)) return null; | |||
There was a problem hiding this comment.
querySelector 자체가 Element, 실패 시 null을 반환하기 때문에
결과를 바로 리턴을 주어도 될 것 같습니다.
main.js
Outdated
| import "./style.scss"; | ||
| import { $ } from "~/utils/querySelector"; | ||
|
|
||
| window.addEventListener("DOMContentLoaded", (e) => { |
There was a problem hiding this comment.
event 객체를 사용하지 않고 있기 때문에
e 를 작성하지 않으셔도 될 것 같습니다.
src/app.js
Outdated
| import Router from "~/router"; | ||
|
|
||
| export default function App($container) { | ||
| this.$container = $container; |
src/app.js
Outdated
| const target = e.target.closest("a"); | ||
| if (!(target instanceof HTMLAnchorElement)) return; | ||
|
|
||
| e.preventDefault(); | ||
| const targetURL = e.target.href.replace(BASE_URL, ""); | ||
| navigate(targetURL); |
There was a problem hiding this comment.
불필요한 변수 혹은 변수로 만들고 사용하지 않는 부분이 있는데
정확한 동작과 값을 파악하면서 구성하면 좋을 것 같습니다.
| const target = e.target.closest("a"); | |
| if (!(target instanceof HTMLAnchorElement)) return; | |
| e.preventDefault(); | |
| const targetURL = e.target.href.replace(BASE_URL, ""); | |
| navigate(targetURL); | |
| e.preventDefault(); | |
| const target = e.target.closest("a"); | |
| if (!(target instanceof HTMLAnchorElement)) return; | |
| navigate(target.href); |
src/scripts/infoPage/infoRoute.js
Outdated
| const target = e.target.closest("a"); | ||
| if(!(target instanceof HTMLAnchorElement)) return null; | ||
|
|
||
| e.preventDefault(); | ||
| const targetURL = target.href.replace(BASE_URL,""); | ||
| navigate(targetURL); |
There was a problem hiding this comment.
동일하게 불필요한 변수 혹은
변수로 잡은 내용을 활용하고 있지 않은 것 같습니다.
| const target = e.target.closest("a"); | |
| if(!(target instanceof HTMLAnchorElement)) return null; | |
| e.preventDefault(); | |
| const targetURL = target.href.replace(BASE_URL,""); | |
| navigate(targetURL); | |
| e.preventDefault(); | |
| const target = e.target.closest("a"); | |
| if (!(target instanceof HTMLAnchorElement)) return; | |
| navigate(target.href); |
src/pages/main.js
Outdated
| const currentYear = new Date().getFullYear(); | ||
| for(let i = currentYear; i >= 1985; i--) { | ||
| const option = document.createElement('option'); | ||
| option.value = i; | ||
| option.innerText = i; | ||
| selectYear.appendChild(option); |
There was a problem hiding this comment.
for문 안에서 반복적으로 append 를 하는 것은 좋지 않을 것 같습니다.
주입해야 할 html을 string으로 만들고 한번 주입하는 것이 좋을 것 같습니다.
로 all year를 제거하고
한번만 주입하면 좋을 것 같습니다.
| const currentYear = new Date().getFullYear(); | |
| for(let i = currentYear; i >= 1985; i--) { | |
| const option = document.createElement('option'); | |
| option.value = i; | |
| option.innerText = i; | |
| selectYear.appendChild(option); | |
| const selectYear = $('.select:last-child') | |
| const currentYear = new Date().getFullYear() | |
| const endYear = 1985 | |
| selectYear.insertAdjacentHTML( | |
| 'afterbegin', | |
| Array.from({ length: currentYear - endYear + 1 }, (_, i) => { | |
| if (i) { | |
| return /*HTML*/ `<option>${currentYear - i}</option>` | |
| } else { | |
| return /*HTML*/ `<option>All Year</option><option>${ | |
| currentYear - i | |
| }</option>` | |
| } | |
| }).join('') | |
| ) |
src/scripts/infoPage/createEl.js
Outdated
| import { $ } from "~/utils/querySelector.js"; | ||
| import detailRes from "./detailRes" | ||
|
|
||
| export default async function createEl () { |
There was a problem hiding this comment.
innerHTML을 사용하여 MovieInfo 에서 만드는 만큼
거기서 필요한 정보를 넣으면 좋을 것 같습니다.
틀을 만드는 부분 따로 만들고
다시 찾아서 거기에 필요한 정보를 넣는 방법은 힘든 방법인 것 같습니다.
There was a problem hiding this comment.
처음에 함수를 movieinfo.js에서 작성하다가, 코드가 너무 길어져 따로 빼내는 과정을 거쳤습니다.
처음에는 무작정 함수로 구현하고 movieinfo에서 호출하는 식으로 작성했었는데, 리뷰를 보니 맞는 말씀이라 어떻게 수정할지 고민했습니다.
innerHTML 안에서 문자열 보간을 사용해서 수정해 봤습니다. 이렇게 하면 createEl.js가 필요 없어지고, 코드는 더 깔끔해진 것 같은데, 익숙치 않아서 시간이 좀 걸렸네요. 이렇게 작성하는 게 더 효율적인 방법일까요?
this.render = async () => {
const details = await detailRes()
this.$container.innerHTML = `
<main class="movieInfo">
<div class="movieInfo__detail">
<div class="detail__container">
<div class="detail__poster">
<img src="${
details.Poster === 'N/A' || details.Response === 'False'
? '/xboxdog.jpg'
: details.Poster.replace('SX300', 'SX700')
}" alt="포스터를 못찾았어요.">
</div>
<div class="container__infos">
<div class="detail__title">
<h2>${details.Title}</h2>
</div>
<div class="detail__rating">
<ul class="rating__container">
${details.Ratings.map(
rating => `
<li class="rating__${rating.Source.replace(
/\s/g,
''
).toLowerCase()}">
${
rating.Source === 'Internet Movie Database'
? '<img src="/imdb.png" class="rating__imdb">'
: ''
}
${
rating.Source === 'Rotten Tomatoes'
? '<img src="/rotten.png" class="rating__rt">'
: ''
}
${
rating.Source === 'Metacritic'
? '<img src="/metacritic.png" class="rating__mt">'
: ''
}
${rating.Value}
</li>
`
).join('')}
</ul>
</div>
<ul class="detail__info">
<li class="info__plot"><h2>PLOT</h2><p>${details.Plot}</p></li>
<li class="info__genre"><h2>GENRE</h2><p>${
details.Genre
}</p></li>
<li class="info__year"><h2>YEAR</h2><p>${details.Year}</p></li>
<li class="info__director"><h2>DIRECTOR</h2><p>${
details.Director
}</p></li>
<li class="info__actors"><h2>ACTORS</h2><p>${
details.Actors
}</p></li>
<li class="info__runtime"><h2>RUNTIME</h2><p>${
details.Runtime
}</p></li>
<li class="info__released"><h2>RELEASED</h2><p>${
details.Released
}</p></li>
<li class="info__country"><h2>COUNTRY</h2><p>${
details.Country
}</p></li>
<li class="info__language"><h2>LANGUAGE</h2><p>${
details.Language
}</p></li>
</ul>
</div>
</div>
</div>
</main>
`
}There was a problem hiding this comment.
처음에 함수를 movieinfo.js에서 작성하다가, 코드가 너무 길어져 따로 빼내는 과정을 거쳤습니다. 처음에는 무작정 함수로 구현하고 movieinfo에서 호출하는 식으로 작성했었는데, 리뷰를 보니 맞는 말씀이라 어떻게 수정할지 고민했습니다. innerHTML 안에서 문자열 보간을 사용해서 수정해 봤습니다. 이렇게 하면 createEl.js가 필요 없어지고, 코드는 더 깔끔해진 것 같은데, 익숙치 않아서 시간이 좀 걸렸네요. 이렇게 작성하는 게 더 효율적인 방법일까요?
this.render = async () => { const details = await detailRes() this.$container.innerHTML = ` <main class="movieInfo"> <div class="movieInfo__detail"> <div class="detail__container"> <div class="detail__poster"> <img src="${ details.Poster === 'N/A' || details.Response === 'False' ? '/xboxdog.jpg' : details.Poster.replace('SX300', 'SX700') }" alt="포스터를 못찾았어요."> </div> <div class="container__infos"> <div class="detail__title"> <h2>${details.Title}</h2> </div> <div class="detail__rating"> <ul class="rating__container"> ${details.Ratings.map( rating => ` <li class="rating__${rating.Source.replace( /\s/g, '' ).toLowerCase()}"> ${ rating.Source === 'Internet Movie Database' ? '<img src="/imdb.png" class="rating__imdb">' : '' } ${ rating.Source === 'Rotten Tomatoes' ? '<img src="/rotten.png" class="rating__rt">' : '' } ${ rating.Source === 'Metacritic' ? '<img src="/metacritic.png" class="rating__mt">' : '' } ${rating.Value} </li> ` ).join('')} </ul> </div> <ul class="detail__info"> <li class="info__plot"><h2>PLOT</h2><p>${details.Plot}</p></li> <li class="info__genre"><h2>GENRE</h2><p>${ details.Genre }</p></li> <li class="info__year"><h2>YEAR</h2><p>${details.Year}</p></li> <li class="info__director"><h2>DIRECTOR</h2><p>${ details.Director }</p></li> <li class="info__actors"><h2>ACTORS</h2><p>${ details.Actors }</p></li> <li class="info__runtime"><h2>RUNTIME</h2><p>${ details.Runtime }</p></li> <li class="info__released"><h2>RELEASED</h2><p>${ details.Released }</p></li> <li class="info__country"><h2>COUNTRY</h2><p>${ details.Country }</p></li> <li class="info__language"><h2>LANGUAGE</h2><p>${ details.Language }</p></li> </ul> </div> </div> </div> </main> ` }
관리적인 측면이나 효율면에서도 좋을 것 같아요
기존 방식은 innerHTML 변경 후 다른 파일에서
해당 엘리먼트를 찾고 거기에 필요한 것들을 추가하는 방식이라
작성도 힘들고 새로운 엘리먼트가 추가 될 때마다 쿼리셀렉트로 찾아주어야 하여
좋지 않았던 것 같습니다.
| resultBox.addEventListener('scroll', () => debouncedHandleScroll(resultBox)); | ||
| if(resultBox.Response === "False") { | ||
| resultBox.removeEventListener('scroll', handleScroll(resultBox)); | ||
| }s |
| const t = $('.select:first-child').value; | ||
| const y = $('.select:nth-child(2)').value; |
There was a problem hiding this comment.
변수가 무슨 값을 가지게 되는지 의미적으로 알 수 있으면 좋을 것 같습니다.
| resultList.appendChild(loading); | ||
| searchPage++ | ||
| const nextPageRes = await fetchMovies(inputText, y, t, searchPage); | ||
| loading.remove() |
There was a problem hiding this comment.
try, catch, final의 final에 로딩 제거를 하는 것이 좋지 않을까 생각이 듭니다.
Vanilla JS로 OMDB API 영화검색 사이트 만들기
💬[데모] : (https://frolicking-trifle-07d151.netlify.app/)
사이트 설명
HOME
INFO
ABOUT
개요:
❗ 필수
❔ 선택
파일 구조 및 기능
🧨문제점 :
info페이지에서 뒤로가기 시, 메인 페이지가 초기화 됩니다. 검색 내역과 데이터가 그대로 남아있어야 사용자도 편하고, 데이터 처리량도 줄어들 것 같습니다. route이벤트 관련된 문제인 것 같은데, 해당 문제점 관련해 서칭중입니다.
무한 스크롤 시 영화 정보가 없음에도, 스크롤을 올렸다가 내리면 이벤트 리스너가 계속 생성되는 현상이 있습니다. 최초 1회 영화정보 없음 표시가 나타나면, 이벤트리스너가 다시 생성되지 않는 로직이 추가되지 않았습니다.
🧠아쉬운 점 :
🎈얻은 것들 :