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단계 - 상세 정보 & UI/UX 개선하기] 윤생(이윤성) 미션 제출합니다. #92

Merged
merged 18 commits into from
Mar 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions cypress/e2e/movie-e2e.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,23 @@ describe("네트워크 에러 확인", () => {
});
});
});

describe("영화 상세 정보(모달창)", () => {

Choose a reason for hiding this comment

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

PR 메세지에 써두긴 했지만, 별점에 대한 테스트도 추후에 보완이 되면 좋을 것 같아요 :)

beforeEach(() => {
cy.visit("http://localhost:8081/");
cy.viewport(1920, 1080);
});

it("영화 목록 중 하나를 선택하여 클릭하면 모달창이 화면에 뜬다.", () => {
cy.intercept(
{
method: "GET",
url: /^https:\/\/api.themoviedb.org\/3\/movie\/*/,
},
{ fixture: "detail.json" }
);

cy.get("movie-item").first().click();
cy.get(".modal-body h3").should("contain", "첫번째 테스트");
});
});
56 changes: 56 additions & 0 deletions cypress/fixtures/detail.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
{
"adult": false,
"backdrop_path": "/a2tys4sD7xzVaogPntGsT1ypVoT.jpg",
"belongs_to_collection": null,
"budget": 35000000,
"genres": [
{ "id": 53, "name": "스릴러" },
{ "id": 35, "name": "코미디" },
{ "id": 27, "name": "공포" },
{ "id": 80, "name": "범죄" }
],
"homepage": "",
"id": 804150,
"imdb_id": "tt14209916",
"original_language": "en",
"original_title": "Cocaine Bear",
"overview": "1985년 마약 밀매업자의 비행기 추락 사고로 인해 비행기에 실려있던 코카인이 조지아주 채터후치 국유림에 떨어지고 코카인을 손에 넣기 위한 마약상들과 관광객들이 코카인을 먹은 흑곰에 의해 처참하게 죽임을 당한다는 내용의 실화를 바탕으로 한 영화",
"popularity": 4435.857,
"poster_path": "/gOnmaxHo0412UVr1QM5Nekv1xPi.jpg",
"production_companies": [
{
"id": 33,
"logo_path": "/8lvHyhjr8oUKOOy2dKXoALWKdp0.png",
"name": "Universal Pictures",
"origin_country": "US"
},
{
"id": 12365,
"logo_path": null,
"name": "Brownstone Productions",
"origin_country": "US"
},
{
"id": 77973,
"logo_path": null,
"name": "Lord Miller Productions",
"origin_country": "US"
}
],
"production_countries": [
{ "iso_3166_1": "IE", "name": "Ireland" },
{ "iso_3166_1": "US", "name": "United States of America" }
],
"release_date": "2023-02-22",
"revenue": 70968453,
"runtime": 95,
"spoken_languages": [
{ "english_name": "English", "iso_639_1": "en", "name": "English" }
],
"status": "Released",
"tagline": "",
"title": "첫번째 테스트",
"video": false,
"vote_average": 6.554,
"vote_count": 493
}
1 change: 1 addition & 0 deletions index.html
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@
</head>
<body>
<app-component></app-component>
<modal-component></modal-component>
</body>
</html>
94 changes: 87 additions & 7 deletions src/components/AppComponent.js

Choose a reason for hiding this comment

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

전체적으로 코드를 다시 보다가 생각난 피드백을 여기에 적을게요~

  1. Index.js 외에도 다른 컴포넌트에서도 웹 컴포넌트 import에 대한 주석은 있었으면 좋겠습니다. 사람들이 무조건 index.js를 통해서 다른 파일들을 본다는 보장이 없으니 안쓰이는 파일로 오해할수도 있을 것 같아요
  2. import transformMovieItemsType from "../util/MovieList"; 이 파일은 더이상 쓰이지 않네요. 파일과 Import 영역 모두 삭제해주세요~
  3. changeButtonDisplayByPage()에 대해서 무한스크롤로 구현을 했지만, 사용자에게 중간중간 '더보기' 버튼이 노출이 되는 것 같아요. 보통의 서비스에서 무한스크롤 UX가 어떻게 되는지 찾아봐서 참고하면 어떨까요? (혹은 intersection observerelement가 어떤 UI를 가지는 지 찾아봐도 좋구요)
  4. handleEvent가 너무 길어서, 이벤트별로 분리를 해보는 건 어떨까요?

Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,38 @@ import HeaderComponent from "./AppHeaderComponent";
import MovieListComponent from "./movie/MovieListComponent";
import MoreButtonComponent from "./element/MoreButtonComponent";
import TitleComponent from "./element/TitleComponent";
import ModalComponent from "./ModalComponent";

import UpScrollButtonComponent from "./element/UpScrollButtonComponent";
import transformMovieItemsType from "../util/MovieList";
import { ACTION, SEARCH_WARNING, TITLE } from "../constants/constants";
import {
ACTION,
ERROR_MESSAGE,
REQUEST_URL,
SCROLL_INVOKE_GAP,
SEARCH_WARNING,
TITLE,
} from "../constants/constants";
import { getRequest, transData } from "../api/handler";
import { urlByActionType } from "../api/url";
import { API_KEY } from "../constants/key";

export default class AppComponent extends CustomComponent {
#nextPage = 1;
#totalPage;
#$movieList;
#$movieListTitle;
#$searchInput;
#scrollThrottleId;
#actionType;

render() {
super.render();

this.#$movieList = this.querySelector("movie-list");
this.#$movieListTitle = this.querySelector("movie-list-title");
this.#$searchInput = this.querySelector("input");
this.#actionType = ACTION.POPULAR;

this.popularListInit();
this.getMovieData(ACTION.POPULAR);
Expand Down Expand Up @@ -49,13 +63,17 @@ export default class AppComponent extends CustomComponent {
}

changeButtonDisplayByPage() {
if (this.#totalPage <= this.#nextPage) {
if (this.isEndOfPage()) {

Choose a reason for hiding this comment

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

함수분리 좋네용 👍

this.querySelector("more-button").classList.add("hide");
return;
}
this.querySelector("more-button").classList.remove("hide");
}

isEndOfPage() {
return this.#totalPage <= this.#nextPage;
}

searchListInit() {
this.#nextPage = 1;

Expand All @@ -82,7 +100,8 @@ export default class AppComponent extends CustomComponent {
switch (e.target.dataset.action) {
case ACTION.POPULAR:
this.popularListInit();
this.getMovieData(ACTION.POPULAR);
this.#actionType = ACTION.POPULAR;
this.getMovieData(this.#actionType);
this.changeMoreButtonAction(ACTION.MORE_POPULAR);
break;
case ACTION.SEARCH:
Expand All @@ -91,16 +110,36 @@ export default class AppComponent extends CustomComponent {
return;
}
this.searchListInit();
this.getMovieData(ACTION.SEARCH);
this.#actionType = ACTION.SEARCH;
this.getMovieData(this.#actionType);
this.changeMoreButtonAction(ACTION.MORE_SEARCH);
break;
case ACTION.MORE_POPULAR:
this.#$movieList.appendNewPage();
this.getMovieData(ACTION.POPULAR);
this.getMovieData(this.#actionType);
break;
case ACTION.MORE_SEARCH:
this.#$movieList.appendNewPage();
this.getMovieData(ACTION.SEARCH);
this.getMovieData(this.#actionType);
break;
case ACTION.UP_SCROLL:
window.scroll({ top: 0, behavior: "smooth" });
break;
case ACTION.DETAIL:
const movieId = e.target.dataset.movieId;
getRequest(
`${REQUEST_URL}/movie/${movieId}?api_key=${API_KEY}&language=ko-KR`
)
.then((res) => {
const modal = document.querySelector("modal-component");
modal.setAttribute("data-item", JSON.stringify(res));

modal.style.display = "flex";
document.body.style.overflow = "hidden";
})
.catch(() => {
alert(ERROR_MESSAGE);
});
break;
Comment on lines +128 to 143

Choose a reason for hiding this comment

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

이부분은 다른 부분과 달리 raw한 느낌이 드는데, 다른 함수로 빼는 방법은 어려웠을까요?
혹은 click이라는 이벤트를 너무 한번에 다 묶으려다보니 성급한 추상화로 인해서 일관성 맞추기가 어려웠을 것 같기도 하네요.

저번에도 비슷한 피드백을 드린 것 같은데 "도메인 관련 로직을 최대한 상위로 빼, 재사용이 가능한 컴포넌트들에게 로직의 책임을 덜 관여하게 하고 싶었음을 의도하고 싶었습니다."의 단점이 드러나는 부분이 아닐까? 싶기도 합니다.

오랜만에 코드를 봤을 때 이 click 이벤트 핸들러는 누구의 이벤트일까? 라는 고민을 많이 하게 만들었던 것 같기도 하네용.

}
});
Expand All @@ -115,10 +154,50 @@ export default class AppComponent extends CustomComponent {
}

this.searchListInit();
this.getMovieData(ACTION.SEARCH);
this.#actionType = ACTION.SEARCH;
this.getMovieData(this.#actionType);
this.changeMoreButtonAction(ACTION.MORE_SEARCH);
}
});

window.addEventListener("keydown", (e) => {
if (e.key === "Escape") {
const modalComponent = document.querySelector("modal-component");

modalComponent.style.display = "none";
document.body.style.overflow = "visible";
}
});

window.addEventListener("scroll", () => {
if (this.isEndOfPage()) return;

this.toggleUpScrollButton();

if (!this.#scrollThrottleId) {
this.#scrollThrottleId = setTimeout(() => {
if (
this.getBoundingClientRect().bottom - window.innerHeight <
SCROLL_INVOKE_GAP
) {
this.#$movieList.appendNewPage();
this.getMovieData(this.#actionType);
}
this.#scrollThrottleId = null;
}, 1000);
Comment on lines +177 to +187

Choose a reason for hiding this comment

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

throttle을 사용하게 된 계기도 궁금하네용

Copy link
Author

Choose a reason for hiding this comment

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

스크롤 이벤트 처리를 this.getBoundingClientRect().bottom - window.innerHeight < SCROLL_INVOKE_GAP(app이 viewport로 부터 얼마나 떨어져 있는지) 확인하므로, 스크롤 이벤트가 발생할 때 쓰로틀링을 걸어주지 않으면 중복되어 호출이 일어나더라구요..!

}
});
}

toggleUpScrollButton() {
const header = document.querySelector("app-header");
const upScrollBtn = document.querySelector("up-scroll-button");

Choose a reason for hiding this comment

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

이런 UX 너무 좋습니다 💯
이 컴포넌트 너무 크다는 생각이 드네용,,!

스크린샷 2023-03-31 오후 2 58 18


if (header.getBoundingClientRect().bottom < 0) {
upScrollBtn.classList.remove("hide");
return;
}
upScrollBtn.classList.add("hide");
}

template() {
Expand All @@ -132,6 +211,7 @@ export default class AppComponent extends CustomComponent {
<more-button></more-button>
</section>
</main>
<up-scroll-button class="hide"></up-scroll-button>
</div>
`;
}
Expand Down
131 changes: 118 additions & 13 deletions src/components/ModalComponent.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,128 @@
import CustomComponent from "../abstracts/CustomComponent";

import StarFilledImg from "../../templates/star_filled.png";
import StarEmptyImg from "../../templates/star_empty.png";
import { OVERVIEW_EMPTY } from "../constants/constants";
export default class ModalComponent extends CustomComponent {

Choose a reason for hiding this comment

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

ModalComponent라는 네이밍과 다르게, 그리고 components의 바로 하위에 있는 컴포넌트 치고는 너무 도메인 종속적이라서 아쉽네요..!

static get observedAttributes() {
return ["data-item"];
}

attributeChangedCallback() {
const modalBody = this.querySelector(".modal-body");
const item = JSON.parse(this.getAttribute("data-item"));
const title = this.querySelector("h3");
const img = this.querySelector("img");
const genre = this.querySelector(".movie-detail-header p");
const desciption = this.querySelector(".movie-detail-description");
const voteAverage = this.querySelector(".movie-detail-vote-average");

title.innerText = item.title;
img.src = `https://image.tmdb.org/t/p/w220_and_h330_face${item.poster_path}`;
img.alt = `별점 ${item.vote_average}`;
genre.innerText = item.genres.map((genre) => genre.name).join(", ");
desciption.innerText = item.overview || OVERVIEW_EMPTY;
voteAverage.innerText = item.vote_average;
modalBody.setAttribute("data-movie-id", item.id);

const rate = Number(localStorage.getItem(`movie_rate_${item.id}`));
this.changeRate(rate);
}

handleEvent() {
this.addEventListener("click", (e) => {
switch (e.target.dataset.action) {
case "modal-close":
this.style.display = "none";
document.body.style.overflow = "visible";
break;
}
});

const customRateContainer = document.querySelector(".custom-rate");
customRateContainer.addEventListener("click", (e) => {
if (!e.target.dataset.actionRate) return;

const modalBody = document.querySelector(".modal-body");
const rate = Number(e.target.dataset.actionRate);

this.changeRate(rate);
localStorage.setItem(`movie_rate_${modalBody.dataset.movieId}`, rate);
});
}

changeRate(rate) {
const selectRateParagraph = document.querySelector(".select-rate-text");
const selectRateScore = document.querySelector(".select-rate-score");
const buttons = document.querySelectorAll(".custom-rate .rate-button");

switch (rate) {
case 0:
selectRateScore.innerText = "";
selectRateParagraph.innerText = "별점을 매겨주세요.";
break;
case 2:
selectRateScore.innerText = "2";
selectRateParagraph.innerText = "최악이에요";
break;
case 4:
selectRateScore.innerText = "4";
selectRateParagraph.innerText = "별로에요";
break;
case 6:
selectRateScore.innerText = "6";
selectRateParagraph.innerText = "보통이에요";
break;
case 8:
selectRateScore.innerText = "8";
selectRateParagraph.innerText = "재미있어요";
break;
case 10:
selectRateScore.innerText = "10";
selectRateParagraph.innerText = "명작이에요";
break;
default:
break;
}

for (const button of buttons) {
const img = button.querySelector("img");

const imgSrc =
+button.dataset.actionRate > rate ? StarEmptyImg : StarFilledImg;
img.src = imgSrc;
}
}

template() {
return `
<div class="modal-body">
<div class="content-header">
<div class="modal-content-header side-padding">
<div class="dummy"></div>
<h3>영화 이름</h3>
<button>닫기 버튼</button>
</div>
<div class="content-body">
<img alt="영화 포스터 이미지">
<div class="movie-details">
<div class="movie-detail-header">
<p>장르</p>
<div>별점</div>
<button class="modal-close-button" data-action="modal-close">X</button>
</div>
<div class="modal-content-body side-padding">
<img class="main-poster-img" alt="영화 포스터 이미지">
<div class="movie-details">
<section>
<div class="movie-detail-header">
<p class="movie-detail-genre"></p>
<img src=${StarFilledImg} />
<div class="movie-detail-vote-average"></div>
</div>
<div class="movie-detail-description"></div>
</section>
<div class="custom-rate">
<h4>내 별점</h4>
<button class="rate-button" data-action-rate="2"><img src=${StarEmptyImg}></button>
<button class="rate-button" data-action-rate="4"><img src=${StarEmptyImg}></button>
<button class="rate-button" data-action-rate="6"><img src=${StarEmptyImg}></button>
<button class="rate-button" data-action-rate="8"><img src=${StarEmptyImg}></button>
<button class="rate-button" data-action-rate="10"><img src=${StarEmptyImg}></button>
<p class="select-rate-score"></p>
<p class="select-rate-text">별점을 매겨주세요.</p>
</div>
</div>
<div> 설명입니다. 설명입니다 </div>
<div> 평점 매기기 </div>
</div>
</div>
`;
}
Expand Down
Loading