[2단계 - 상세 정보 & UI/UX 개선하기] 디움 미션 제출합니다. #296
[2단계 - 상세 정보 & UI/UX 개선하기] 디움 미션 제출합니다. #296DongEun02 wants to merge 31 commits intowoowacourse:dongeun02from
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough이 PR은 영화 애플리케이션의 Step 2 기능을 추가합니다. 영화 상세 정보 모달 컴포넌트 구현, 별점 평가 시스템 추가, 페이지 버튼에서 무한 스크롤로의 전환이 포함됩니다. 새로운 Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
cys4585
left a comment
cys4585
left a comment
There was a problem hiding this comment.
안녕하세요 디움~!
2단계 미션 고생하셨습니다!
우선, 1단계 피드백을 잘 반영한 부분이 눈에 띄어요. 특히 1단계 구조를 유지하면서 모달/무한스크롤/별점을 추가한 경험에서 "역할이 명확하면 확장이 쉽다"는 걸 직접 느낀 것도 좋고, "더보기 버튼의 의존도가 높았다"는 체감도 좋은 경험이 되었을 것 같습니다!
배포 링크에서 동작을 확인해봤는데, 몇 가지 이슈가 있었어요.
- 모달의 X 버튼, 별 아이콘이 안 보여요. 이미지 리소스를 못 불러오는 것 같아요.
- 모달을 여러 번 열면 DOM에 모달이 계속 쌓여요. 개발자 도구에서
.modal-background요소가 누적되는 걸 확인했어요. - 검색 결과가 적은 영화를 검색해서 무한 스크롤을 테스트해보면, 마지막 페이지를 넘어서도 계속 API를 호출해요.
- 네트워크를 느리게 해서 로딩 상태를 확인해봤는데, 스켈레톤이 보이지 않았어요.
아키텍처
디움의 코드를 보니 이런 단방향 흐름으로 설계한 것 같아요.
DOM (이벤트 발생: load, click, submit, scroll, keydown)
↓
main.ts (이벤트 수신 + DOM에서 값 추출)
↓
controllerHandlers.ts (비즈니스 로직 + API 호출 + 에러 처리)
↓
renderHandlers.ts (적절한 UI 클래스에 데이터 전달)
↓
UI 클래스 — Header, MainTitle, MovieList, Modal (DOM 조작)
↓
DOM (화면 갱신)
각 레이어의 역할:
- main.ts: 이벤트를 받아서 필요한 값을 추출하고 controller에 위임
- controllerHandlers: 비즈니스 규칙 판단, API/저장소 호출, 에러 처리
- renderHandlers: 데이터를 받아서 적절한 UI 클래스에 전달
- UI 클래스: 받은 데이터로 DOM을 직접 조작
(우선, 제가 디움의 의도를 오해한거라면 알려주세요!)
근데 코드를 보면 위 원칙이 일관되게 지켜지지 않는 부분들이 있어요. 아래에서 짚는 포인트들은 대부분 "이 단방향 흐름이 깨지는 지점"이라 봐주시면 될 것 같아요.
핵심 포인트
레이어 역할 경계
main.ts가 이벤트 수신 + 값 추출만 하고 controller에 위임하는 게 원칙인데, 실제로는 비즈니스 로직이 main에 많이 남아 있어요.
- 검색 핸들러: 빈 검색어 분기(
searchMovie === ""→controlInitialMovies), page 리셋(page = 1) - 로고 클릭:
page = 1,searchMovie = ""리셋 - 무한 스크롤:
page += 1 - 별점:
setRating → closeModal → controlModal3단계를 main이 직접 조합 - ...
load 이벤트는 await controlInitialMovies(page) 한 줄로 깔끔하게 controller에 위임하고 있는데, 이게 원래 의도한 패턴이라면, 다른 핸들러들도 같은 원칙으로 정리할 수 있을 것 같아요. 그렇게되면 아주 읽기 좋은 코드가 될 것 같습니다!
그리고 Modal(UI 클래스)이 getRating을 직접 호출해서 localStorage에 접근하고 있는데, 이건 아래쪽 레이어에서 위로 거슬러 올라가는 거예요. 다른 데이터는 controller → render → UI로 내려오는데 별점만 UI가 직접 가져오고 있어요. 미션 요구사항에 "서버 API로 교체할 수 있는 구조"라고 되어 있는데, 지금 구조에서 교체하려면 어떤 파일들을 수정해야 하는지 한번 따져보면 좋을 것 같습니다.
PR 본문 질문 답변
1. z-index
근본 원인은 overlay의 HTML 구조에 있다고 봅니다. overlay의 역할이 뭔지 생각해보고 거기에 맞춰 수정해보면 어떨까 싶습니다. 배경을 어둡게 하는 시각 효과라면, 인터랙티브 요소를 덮지 않게 구조를 잡거나 pointer-events: none으로 클릭을 통과시키는 방법이 있을 것 같아요.
2. Modal의 getRating
위에서 짚은 레이어 역할 경계와 비슷한 맥락인데요. 영화 상세 데이터는 controller에서 가져와서 Modal에 넘겨주는데, 별점만 Modal이 직접 localStorage에 접근하고 있잖아요. 디움이 생각하는 "UI 컴포넌트가 어디까지 데이터에 접근해도 되는지"의 기준이 궁금합니다.
3. renderHandlers 전역 인스턴스
현재 규모에서는 문제없어보입니다. renderHandlers의 역할이 UI를 컨트롤하는 거니까, UI 인스턴스를 renderHandlers가 직접 갖고 있는 건 자연스럽다고 생각해요.
디움의 생각이 궁금합니다. 어떤 부분 때문에 고민을 하셨는지 궁금해요.
4. 전역 변수 상태 관리
디움은 "논의해보고 싶다"고 했는데, 디움의 의견이 먼저 궁금해요! 지금 구조에서 불편한 점이 있었는지, 어떤 방향으로 개선하고 싶은지 공유해주면 더 구체적인 논의가 가능할 것 같아요.
E2E 테스트
주요 유저 시나리오를 잘 커버하고 있는 것 같아요.
빠진 시나리오 몇 개 짚어보자면:
- 모달 X 버튼 클릭으로 닫기
- 검색 결과 없음
- 자세히 보기 버튼으로 모달 열기
"유저가 이 앱에서 할 수 있는 모든 행동"을 나열해보고, 각각에 대한 테스트가 있는지 점검해보면 좋을 것 같아요~
| // 무한 스크롤 | ||
| window.addEventListener("scroll", async () => { | ||
| if (window.innerHeight + window.scrollY >= document.body.scrollHeight - 1) { | ||
| page += 1; | ||
| await appendNextPageMovies(page, searchMovie); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
"마지막 페이지 넘어서도 계속 API 호출" 이슈의 원인이 여기인 것 같네요~!
- total_pages 체크가 빠져 있고,
- API 실패 시 page 복구도 빠져 있네요
| modalBackground.classList.remove("active"); | ||
| }); |
There was a problem hiding this comment.
X 버튼 클릭 시에는 closeModal()을 호출하는데, ESC에서는 직접 DOM을 조작하고 있네요. 같은 "모달 닫기"인데 경로가 다른 건 의도한 걸까요??
There was a problem hiding this comment.
Commit
만들어놓은 closeModal을 사용할 생각을 못했던 것 같습니다. 같은 메서드를 사용하는 방식으로 수정했습니다!
| setRating(id, rating); | ||
| closeModal(modalBackground); | ||
| await controlModal(id); |
There was a problem hiding this comment.
다른 로직들(영화 목록, 상세)은 controllerHandlers를 거치는데, 별점만 main에서 직접 처리하고 있네요. 이렇게 한 이유가 있나요?
그리고 별점은 localStorage에 저장하는 건데, closeModal로 모달을 닫았다가, controlModal(id)로 서버에서 영화 정보를 다시 가져오고 있네요. 이것도 의도가 따로 있었던걸까요?
There was a problem hiding this comment.
현재 제 구조는 컨트롤러에서 비즈니스 로직을 담당하지만, 해당 부분은 main.ts에서 담당하도록 되어있어 역할에 맞지 않는 것 같습니다! await controlModal을 먼저 구현한 후, 나중에 setRating과 closeModal을 구현하다보니 컨트롤러로 분리할 생각은 하지 못하고 controlModal을 호출하기 전에 별점을 등록하고, 모달을 닫았다고 다시 열면 되겠구나라는 생각만 하여 main.ts에 추가했습니다.
모달을 닫았다고 다시 연 이유는 별점을 등록해도 결과가 바로 반영이 되지 않아 해당 오류를 수정하기 위해 모달을 닫았다가 다시 여는 방식을 선택했습니다. 현재는 별점 부분만 업데이트하는 방식으로 수정했습니다.
| export function openModal(data: MovieDetailResponse) { | ||
| const modal = new Modal(data); | ||
| modal.renderModal(); | ||
| } | ||
|
|
||
| export function closeModal(element: HTMLElement) { | ||
| element.classList.remove("active"); | ||
| } |
There was a problem hiding this comment.
동작 확인에서 짚은 "모달 DOM 누적" 이슈의 원인이 여기인 것 같아요~!
| const rating = getRating(this.movieInfo.id); | ||
| const scores = [2, 4, 6, 8, 10]; |
There was a problem hiding this comment.
영화 상세 데이터는 controllerHandlers에서 가져와서 Modal에 넘겨주는데, 별점은 Modal이 직접 가져오는 이유가 있을까요?
미션 요구사항에 "서버 API로 교체할 수 있는 구조"라고 되어 있는데, 지금 구조에서 서버 API로 교체해야 한다고 하면, 어떻게 대응하실려고 하셨는지, 디움의 생각이 궁금합니다!
There was a problem hiding this comment.
해당 로직에서 이 방식은 아닌 것 같다는 생각이 들어서 2번 질문으로 남겼습니다. UI에서 직접 데이터를 가져오는 방식은 잘못됐다고 생각합니다. 현재는 컨트롤러에서 데이터를 가져와 값을 넘기도록 수정하였습니다.
변경하기 전 구조에서는 getRating 메서드의 내부 로직만 변경하는 방식으로 대응하려고 생각했습니다. getRating 메서드에서 API를 호출하여 데이터를 받아오거나 API를 호출하여 데이터를 받아오는 메서드를 호출하여 Modal로 값을 전달하려고 하였습니다!
| src="${score <= (rating ?? 0) ? "./src/images/star_filled.png" : "./src/images/star_empty.png"}" | ||
| data-id="${score}" |
There was a problem hiding this comment.
동작 확인에서 짚은 "모달 이미지 안 보임" 이슈의 원인이 여기인 것 같아요.
There was a problem hiding this comment.
이미지를 import 후 사용하는 방식으로 수정했습니다!
| win.dispatchEvent(new Event("scroll")); | ||
| }); |
There was a problem hiding this comment.
cy.scrollTo("bottom")처럼 유저 행동 기반으로 테스트하는 방법이 실제 사용자 시나리오 테스트를 한다는 e2e 역할에 부합하지 않을까 생각이 듭니다!
(이유가 있었다면 말씀해주시면 감사하겠습니다!)
There was a problem hiding this comment.
사실 이번 미션에서 E2E 테스트는 AI를 사용해서 구현했습니다. E2E 테스트보다는 역할 분리, DOM 조작 등에 비중을 두고 싶었습니다....
유저 행동 기반으로 테스트하도록 수정했습니다!
| doc.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape" })); | ||
| }); |
There was a problem hiding this comment.
scroll test와 동일한 피드백 드립니다! (사용자 시나리오 기반)
DongEun02
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/styles/main.css (1)
146-159:⚠️ Potential issue | 🟠 Major오버레이 계층이 콘텐츠 인터랙션을 가릴 수 있습니다.
Line 152의
.overlay(z-index: 100)가 Line 155-159의 콘텐츠(z-index: 2)보다 위에 있어, 같은 컨텍스트에서는 텍스트/클릭 영역을 덮을 수 있습니다.
오버레이를 배경 용도로만 쓴다면 z-index를 낮추거나pointer-events처리, 혹은 콘텐츠 레이어를 더 위로 올리는 방식으로 계층을 재정렬해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/main.css` around lines 146 - 159, The .overlay (z-index: 100) sits above .top-rated-container (z-index: 2) and can block interactions; fix by reordering stacking/interaction: either lower .overlay z-index to sit behind content, add pointer-events: none to .overlay if it should be non-interactive, or raise .top-rated-container’s stacking context (ensure it has position set and increase its z-index) so interactive elements are above the overlay; update the CSS rules for .overlay and/or .top-rated-container accordingly.
♻️ Duplicate comments (2)
src/main.ts (1)
61-65:⚠️ Potential issue | 🟠 Major무한 스크롤이 마지막 페이지 이후에도 계속 호출될 수 있습니다.
여기서는
page만 먼저 증가시키고 종료 조건이나 요청 중 가드가 없어서, 바닥에 머무는 동안 연속 호출이 발생하고 실패 시에도 페이지 상태가 어긋납니다.total_pages나hasNext/isFetching를 어디서 관리할지 먼저 정리해보면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 61 - 65, The scroll handler can trigger multiple calls past the last page because it increments page immediately without guards; update the logic in the window "scroll" listener and appendNextPageMovies flow to prevent overfetching by: 1) introducing an isFetching flag (or hasNext) checked at the top of the handler to short-circuit while a request is in progress; 2) fetching/reading total_pages (or server-provided hasNext) and only incrementing page and calling appendNextPageMovies(page, searchMovie) when page < total_pages (or hasNext is true); and 3) ensuring page is only advanced on successful fetch (or rolled back on failure) inside appendNextPageMovies to keep state consistent. Reference: the window.addEventListener("scroll", ...) handler and the appendNextPageMovies function.src/features/handler/renderHandlers.ts (1)
46-53:⚠️ Potential issue | 🟠 Major모달 노드가 누적될 가능성이 여전히 있습니다.
Line 46-49에서 모달을 매번 새로 생성하고, Line 51-53에서는 클래스만 제거하고 있어 DOM 정리가 보장되지 않습니다. 모달 재오픈/평점 갱신 흐름에서 중복 노드·이벤트가 쌓일 수 있어요.
모달 루트를 단일 인스턴스로 재사용하거나, 재렌더 전 기존 노드를 교체/정리하는 방식으로 정리해보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/handler/renderHandlers.ts` around lines 46 - 53, The current openModal creates a new Modal instance each time and closeModal only removes the "active" class, causing DOM nodes and event listeners to accumulate; update the logic to reuse a single modal root or ensure full teardown before creating a new modal: modify openModal (and Modal.renderModal) to check for an existing modal container (e.g., a singleton modal root element or a stored Modal instance) and replace or re-render into that root instead of appending a new node, and update closeModal to fully remove the modal element from the DOM and call a cleanup/destroy method on the Modal instance to remove any attached event listeners and references (reference: functions openModal, closeModal, class Modal, and method renderModal).
🧹 Nitpick comments (3)
README.md (1)
108-108: 표기 통일을 위해메세지를메시지로 수정해주세요.사용자 노출 문구/문서 용어는 맞춤법 일관성을 맞추는 편이 좋습니다.
✏️ 수정 제안
-- API 호출 오류 시 오류 메세지를 화면에 출력한다. +- API 호출 오류 시 오류 메시지를 화면에 출력한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 108, The README contains a misspelling in the sentence "API 호출 오류 시 오류 메세지를 화면에 출력한다."—change "메세지" to the correct "메시지" (resulting text: "API 호출 오류 시 오류 메시지를 화면에 출력한다.") and scan the README for any other occurrences of "메세지" to replace with "메시지" for consistent terminology.src/features/UI/MovieCard.ts (1)
13-13: HTML 속성 값에 따옴표 사용을 권장합니다.
data-id=${this.movie.id}에서 속성 값이 따옴표로 감싸지지 않았습니다. 숫자 ID라서 현재는 문제없이 동작하지만, HTML 속성 값은 항상 따옴표로 감싸는 것이 권장됩니다.이 부분을 어떻게 수정하면 좋을지 생각해 보시겠어요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/UI/MovieCard.ts` at line 13, The template returned in MovieCard (the method returning `/*html*/ \`<div class="item" data-id=${this.movie.id}>\``) should wrap the attribute value in quotes for valid HTML; change it to `data-id="${this.movie.id}"` (or `data-id="${String(this.movie.id)}"` if you want explicit conversion) so the `this.movie.id` is quoted in the rendered markup.src/styles/modal.css (1)
134-137:hr는 모달 범위로 한정해두는 편이 안전합니다.지금 선택자는 전역이라 이 스타일시트가 로드되는 모든 화면의 구분선에 영향을 줄 수 있습니다.
.modal hr처럼 범위를 좁혀두면 이후 사이드이펙트를 줄이기 쉽습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/modal.css` around lines 134 - 137, 현재 전역 선택자 hr이 모달 전용 CSS 파일에 있어 다른 화면의 구분선에 영향을 줄 수 있으니 선택자를 모달 범위로 제한하세요: modal 스타일에 적용된 hr 규칙을 전역 hr { ... }에서 .modal hr { opacity: 1; width: 100%; }처럼 변경하여 해당 스타일이 모달 내부에만 적용되도록 수정하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 100: Fix the typo in the README where the phrase "영화 상제 정보" appears;
replace it with "영화 상세 정보" so the sentence reads "모달 창에 영화 상세 정보를 표시한다." and
ensure any other occurrences of "상제" in the README are corrected to "상세".
In `@src/constants/constant.ts`:
- Around line 14-20: Fix the typo in MOVIE_RATING by changing "최악이예요" to
"최악이에요", and make MOVIE_RATING strongly typed to avoid unsafe assertions in
Modal.ts: declare MOVIE_RATING with a precise type (e.g., a Record with keys
2|4|6|8|10 or export it as a const literal and then export type MovieRatingKey =
keyof typeof MOVIE_RATING), then update Modal.ts to use MovieRatingKey (or keyof
typeof MOVIE_RATING) instead of using a manual "as keyof typeof MOVIE_RATING"
assertion so the compiler enforces valid keys.
In `@src/features/handler/controllerHandlers.ts`:
- Around line 22-23: The code calls showHeader(data.results[0]) without guarding
against an empty results array; update the controller handler to check that data
&& Array.isArray(data.results) && data.results.length > 0 before calling
showHeader, and if the array is empty call an alternative branch (e.g., skip
showHeader, call showEmptyHeader or render a fallback message) then always call
showMovieList(data) as appropriate; modify the logic around the showHeader and
showMovieList calls to use these checks and fallback behavior to avoid passing
undefined to showHeader.
In `@src/features/UI/Header.ts`:
- Around line 23-31: The current assignment to
this.backgroundContainer.innerHTML injects untrusted values (movie.title and
user-controlled searchMovie) directly, creating DOM XSS risks; instead build the
DOM using safe DOM APIs: create elements (or use this.renderImage() for safe
HTML only), set text nodes via element.textContent for movie.title and button
text, set attributes like data-id via element.setAttribute, and insert
vote_average using Number formatting rather than raw HTML; repeat the same fix
for the similar block referenced at lines 46-57 so no user/API strings are
interpolated into innerHTML.
In `@src/features/UI/Modal.ts`:
- Around line 23-27: The star rating uses plain <img> elements (class "star",
data-id="${score}") which are not keyboard- or screenreader-accessible; replace
each star img with an interactive element (preferably a <button> or a group of
radio inputs) inside the Modal component's star-rendering logic so it supports
focus, keyboard events (Enter/Space, Arrow keys if radio group), and ARIA state.
Ensure the new control preserves the score identifier (data-id or value), sets
appropriate semantics (role="radio"/aria-checked or native input/button
semantics), manages focus/tabindex, and updates the same rating state; apply the
same conversion to the other star block referenced (lines 59-71) so both
displays are accessible.
- Around line 31-82: The current modal creation uses
modalContainer.insertAdjacentHTML(... "beforeend") which accumulates duplicate
elements (id="modalBackground", id="closeModal") each open; before inserting the
new modal HTML, detect and remove or replace any existing modal background
element (querySelector for "#modalBackground" or ".modal-background") so you
don't accumulate hidden modals, or alternatively use modalContainer.innerHTML =
`...` to replace contents; ensure the close handler (element with id
"closeModal") removes the modal node from the DOM (or removes the background
element) rather than only toggling the "active" class so subsequent opens start
from a clean state.
- Around line 36-37: Add an accessible name to the close button and mark the
image as decorative: update the element with id "closeModal" (class
"close-modal") to include an appropriate aria-label (e.g., aria-label="Close
modal") and make the nested <img> decorative by giving it an empty alt attribute
("") so screen readers announce the button purpose rather than the image.
- Around line 34-77: The template currently injects untrusted movieInfo fields
directly into the HTML string (this.movieInfo.title, this.movieInfo.genres,
this.movieInfo.overview and the computed category string), which allows HTML
injection; instead build the modal DOM with createElement/appendChild (or set
innerHTML only for trusted fragments like starsHtml if truly safe) and set text
nodes via textContent for title, category/genre list (join genre names into a
plain string and assign to textContent), overview (use textContent with the
fallback string), and any rating labels (use MOVIE_RATING lookup but assign its
value to textContent). Locate the modal construction using this.movieInfo,
starsHtml and MOVIE_RATING in Modal.ts and replace the inline template injection
for textual fields with explicit DOM nodes whose textContent is set to the
sanitized values.
In `@src/main.ts`:
- Around line 126-137: target.closest(".star") can match decorative stars inside
.rate that lack data-id, producing NaN for rating; update the click handling
around target.closest(".star") so it only selects stars with a data-id (e.g.,
use selector like '.star[data-id]' or check that star.dataset.id exists before
parsing), and if missing return early instead of calling
setRating/closeModal/controlModal; ensure you reference the same
modal/modalBackground flow and use the id/rating only when star.dataset.id is
defined.
In `@src/styles/modal.css`:
- Around line 120-128: The .detail-content CSS contains invalid declarations:
replace the invalid font-style: Light with a valid font-style (e.g.,
normal/italic/oblique) and rely on font-weight: 300 for light weight, and change
font-family: "Montserrat" to include a generic fallback (e.g., 'Montserrat',
sans-serif) to satisfy stylelint; apply the same fixes to the other occurrences
referenced (lines 166-167) so CI/stylelint stops failing and browsers don't
ignore these rules.
In `@types/types.ts`:
- Line 20: The genres property is currently typed as a single-element tuple `[{
name: string }]` causing runtime mismatches; change the type of the `genres`
property in types/types.ts (the `genres` field) from a single-element tuple to a
variable-length array type (e.g., use `{ name: string }[]` or `Array<{ name:
string }>`), so components like Modal.ts and tests that expect multiple genres
work correctly.
---
Outside diff comments:
In `@src/styles/main.css`:
- Around line 146-159: The .overlay (z-index: 100) sits above
.top-rated-container (z-index: 2) and can block interactions; fix by reordering
stacking/interaction: either lower .overlay z-index to sit behind content, add
pointer-events: none to .overlay if it should be non-interactive, or raise
.top-rated-container’s stacking context (ensure it has position set and increase
its z-index) so interactive elements are above the overlay; update the CSS rules
for .overlay and/or .top-rated-container accordingly.
---
Duplicate comments:
In `@src/features/handler/renderHandlers.ts`:
- Around line 46-53: The current openModal creates a new Modal instance each
time and closeModal only removes the "active" class, causing DOM nodes and event
listeners to accumulate; update the logic to reuse a single modal root or ensure
full teardown before creating a new modal: modify openModal (and
Modal.renderModal) to check for an existing modal container (e.g., a singleton
modal root element or a stored Modal instance) and replace or re-render into
that root instead of appending a new node, and update closeModal to fully remove
the modal element from the DOM and call a cleanup/destroy method on the Modal
instance to remove any attached event listeners and references (reference:
functions openModal, closeModal, class Modal, and method renderModal).
In `@src/main.ts`:
- Around line 61-65: The scroll handler can trigger multiple calls past the last
page because it increments page immediately without guards; update the logic in
the window "scroll" listener and appendNextPageMovies flow to prevent
overfetching by: 1) introducing an isFetching flag (or hasNext) checked at the
top of the handler to short-circuit while a request is in progress; 2)
fetching/reading total_pages (or server-provided hasNext) and only incrementing
page and calling appendNextPageMovies(page, searchMovie) when page < total_pages
(or hasNext is true); and 3) ensuring page is only advanced on successful fetch
(or rolled back on failure) inside appendNextPageMovies to keep state
consistent. Reference: the window.addEventListener("scroll", ...) handler and
the appendNextPageMovies function.
---
Nitpick comments:
In `@README.md`:
- Line 108: The README contains a misspelling in the sentence "API 호출 오류 시 오류
메세지를 화면에 출력한다."—change "메세지" to the correct "메시지" (resulting text: "API 호출 오류 시
오류 메시지를 화면에 출력한다.") and scan the README for any other occurrences of "메세지" to
replace with "메시지" for consistent terminology.
In `@src/features/UI/MovieCard.ts`:
- Line 13: The template returned in MovieCard (the method returning `/*html*/
\`<div class="item" data-id=${this.movie.id}>\``) should wrap the attribute
value in quotes for valid HTML; change it to `data-id="${this.movie.id}"` (or
`data-id="${String(this.movie.id)}"` if you want explicit conversion) so the
`this.movie.id` is quoted in the rendered markup.
In `@src/styles/modal.css`:
- Around line 134-137: 현재 전역 선택자 hr이 모달 전용 CSS 파일에 있어 다른 화면의 구분선에 영향을 줄 수 있으니
선택자를 모달 범위로 제한하세요: modal 스타일에 적용된 hr 규칙을 전역 hr { ... }에서 .modal hr { opacity: 1;
width: 100%; }처럼 변경하여 해당 스타일이 모달 내부에만 적용되도록 수정하세요.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 280a4fbd-36c8-4b83-a598-b2358cb1f40c
⛔ Files ignored due to path filters (1)
src/images/Star.pngis excluded by!**/*.png
📒 Files selected for processing (21)
.github/workflows/deploy.ymlREADME.md__tests__/api.test.tscypress/e2e/spec.cy.tsindex.htmlsrc/constants/constant.tssrc/features/UI/Header.tssrc/features/UI/Modal.tssrc/features/UI/MovieCard.tssrc/features/api/fetchMovieDetailApi.tssrc/features/api/fetchMoviesApi.tssrc/features/handler/controllerHandlers.tssrc/features/handler/renderHandlers.tssrc/main.tssrc/styles/colors.csssrc/styles/main.csssrc/styles/modal.csssrc/styles/thumbnail.csssrc/utils/getRating.tssrc/utils/setRating.tstypes/types.ts
💤 Files with no reviewable changes (1)
- src/styles/colors.css
|
|
||
| ### 모달 UI | ||
|
|
||
| - 모달 창에 영화 상제 정보를 표시한다. |
There was a problem hiding this comment.
오탈자 수정이 필요합니다 (상제 → 상세).
Line 100의 영화 상제 정보는 영화 상세 정보가 맞습니다.
✏️ 수정 제안
-- 모달 창에 영화 상제 정보를 표시한다.
+- 모달 창에 영화 상세 정보를 표시한다.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - 모달 창에 영화 상제 정보를 표시한다. | |
| - 모달 창에 영화 상세 정보를 표시한다. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 100, Fix the typo in the README where the phrase "영화 상제
정보" appears; replace it with "영화 상세 정보" so the sentence reads "모달 창에 영화 상세 정보를
표시한다." and ensure any other occurrences of "상제" in the README are corrected to
"상세".
| export const MOVIE_RATING = { | ||
| 2: "최악이예요", | ||
| 4: "별로예요", | ||
| 6: "보통이에요", | ||
| 8: "재미있어요", | ||
| 10: "명작이에요", | ||
| }; |
There was a problem hiding this comment.
맞춤법 확인이 필요합니다.
Line 15의 "최악이예요"는 "최악이에요"가 올바른 표기입니다. 받침이 있는 명사 뒤에는 "이에요"를 사용합니다.
또한, MOVIE_RATING 객체에 타입을 명시하면 타입 안전성을 높일 수 있습니다. 현재 Modal.ts에서 as keyof typeof MOVIE_RATING으로 타입 단언을 사용하고 있는데, 이 부분을 어떻게 개선할 수 있을지 고민해 보시겠어요?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/constants/constant.ts` around lines 14 - 20, Fix the typo in MOVIE_RATING
by changing "최악이예요" to "최악이에요", and make MOVIE_RATING strongly typed to avoid
unsafe assertions in Modal.ts: declare MOVIE_RATING with a precise type (e.g., a
Record with keys 2|4|6|8|10 or export it as a const literal and then export type
MovieRatingKey = keyof typeof MOVIE_RATING), then update Modal.ts to use
MovieRatingKey (or keyof typeof MOVIE_RATING) instead of using a manual "as
keyof typeof MOVIE_RATING" assertion so the compiler enforces valid keys.
| showHeader(data.results[0]); | ||
| showMovieList(data); |
There was a problem hiding this comment.
data.results[0] 접근 전 빈 배열 방어가 필요합니다.
Line 22에서 결과가 비어 있으면 undefined가 showHeader로 전달됩니다. 인기 목록이 비정상/빈 응답일 때 초기 렌더가 깨질 수 있어요.
data.results.length를 먼저 확인하고, 비어 있으면 헤더를 건너뛰거나 대체 UI를 보여주는 분기 추가를 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/handler/controllerHandlers.ts` around lines 22 - 23, The code
calls showHeader(data.results[0]) without guarding against an empty results
array; update the controller handler to check that data &&
Array.isArray(data.results) && data.results.length > 0 before calling
showHeader, and if the array is empty call an alternative branch (e.g., skip
showHeader, call showEmptyHeader or render a fallback message) then always call
showMovieList(data) as appropriate; modify the logic around the showHeader and
showMovieList calls to use these checks and fallback behavior to avoid passing
undefined to showHeader.
| this.backgroundContainer.innerHTML = /*html*/ ` | ||
| ${this.renderImage()} | ||
| <div class="top-rated-movie"> | ||
| <div class="rate"> | ||
| <img src="${starImg}" class="star" /> | ||
| <span class="rate-value">${movie.vote_average.toFixed(1)}</span> | ||
| </div> | ||
| <div class="title">${movie.title}</div> | ||
| <button class="primary detail">자세히 보기</button> | ||
| <button class="primary detail" data-id="${movie.id}">자세히 보기</button> |
There was a problem hiding this comment.
원본 데이터를 HTML 문자열에 그대로 넣고 있습니다.
movie.title은 외부 API 값이고 searchMovie는 사용자 입력인데, 둘 다 innerHTML 템플릿에 바로 들어가서 DOM XSS 지점이 됩니다. 텍스트와 입력값은 DOM API(textContent, value)로 넣거나 escape 처리가 필요합니다.
Also applies to: 46-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/UI/Header.ts` around lines 23 - 31, The current assignment to
this.backgroundContainer.innerHTML injects untrusted values (movie.title and
user-controlled searchMovie) directly, creating DOM XSS risks; instead build the
DOM using safe DOM APIs: create elements (or use this.renderImage() for safe
HTML only), set text nodes via element.textContent for movie.title and button
text, set attributes like data-id via element.setAttribute, and insert
vote_average using Number formatting rather than raw HTML; repeat the same fix
for the similar block referenced at lines 46-57 so no user/API strings are
interpolated into innerHTML.
| <img | ||
| src="${score <= (rating ?? 0) ? star_filled : star_empty}" | ||
| data-id="${score}" | ||
| class="star" | ||
| /> |
There was a problem hiding this comment.
별점 컨트롤이 마우스 클릭에만 의존합니다.
인터랙티브 요소가 <img>라 키보드 포커스와 스크린리더 조작이 어렵습니다. 버튼이나 라디오 그룹처럼 의미가 있는 컨트롤로 바꾸면 접근성과 상태 표현을 같이 정리할 수 있습니다.
Also applies to: 59-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/UI/Modal.ts` around lines 23 - 27, The star rating uses plain
<img> elements (class "star", data-id="${score}") which are not keyboard- or
screenreader-accessible; replace each star img with an interactive element
(preferably a <button> or a group of radio inputs) inside the Modal component's
star-rendering logic so it supports focus, keyboard events (Enter/Space, Arrow
keys if radio group), and ARIA state. Ensure the new control preserves the score
identifier (data-id or value), sets appropriate semantics
(role="radio"/aria-checked or native input/button semantics), manages
focus/tabindex, and updates the same rating state; apply the same conversion to
the other star block referenced (lines 59-71) so both displays are accessible.
| <div class="modal-background active" id="modalBackground"> | ||
| <div class="modal" data-id="${this.movieInfo.id}"> | ||
| <button class="close-modal" id="closeModal"> | ||
| <img src=${modal_close} /> | ||
| </button> | ||
| <div class="modal-container"> | ||
| <div class="modal-image"> | ||
| <img | ||
| src="https://image.tmdb.org/t/p/original/${this.movieInfo.poster_path}" | ||
| /> | ||
| </div> | ||
| <div class="modal-description"> | ||
| <div class="movie-info"> | ||
| <h2>${this.movieInfo.title}</h2> | ||
| <p class="category"> | ||
| ${this.movieInfo.release_date.split("-")[0]} · ${this.movieInfo.genres.map((genre) => genre.name).join(", ")} | ||
| </p> | ||
| <p class="rate"> | ||
| <span class="rate-label">평균 </span> | ||
| <img src=${star_filled} class="star" /><span | ||
| >${this.movieInfo.vote_average.toFixed(1)}</span | ||
| > | ||
| </p> | ||
| </div> | ||
| <hr /> | ||
| <div class="my-rating"> | ||
| <span class="detail-label">내 별점</span> | ||
| <div class="rating-container"> | ||
| <div class="stars"> | ||
| ${starsHtml} | ||
| </div> | ||
| ${ | ||
| rating | ||
| ? `<span class="detail-label">${MOVIE_RATING[rating as keyof typeof MOVIE_RATING]}</span> | ||
| <span class="my-rating-value">(${rating}/10)</span>` | ||
| : `<span class="detail-label">별점을 선택하세요</span>` | ||
| } | ||
| </div> | ||
| </div> | ||
| <hr /> | ||
| <p class="detail-content"> | ||
| <span class="detail-label">줄거리</span> | ||
| ${this.movieInfo.overview !== "" ? this.movieInfo.overview : "줄거리가 존재하지 않습니다."} | ||
| </p> |
There was a problem hiding this comment.
외부 API 값을 HTML 문자열에 그대로 주입하고 있습니다.
title, genres, overview가 insertAdjacentHTML로 바로 들어가서 예기치 않은 HTML이 섞이면 그대로 DOM으로 해석됩니다. 상세 텍스트는 노드를 만든 뒤 textContent로 채우는 쪽이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/UI/Modal.ts` around lines 34 - 77, The template currently
injects untrusted movieInfo fields directly into the HTML string
(this.movieInfo.title, this.movieInfo.genres, this.movieInfo.overview and the
computed category string), which allows HTML injection; instead build the modal
DOM with createElement/appendChild (or set innerHTML only for trusted fragments
like starsHtml if truly safe) and set text nodes via textContent for title,
category/genre list (join genre names into a plain string and assign to
textContent), overview (use textContent with the fallback string), and any
rating labels (use MOVIE_RATING lookup but assign its value to textContent).
Locate the modal construction using this.movieInfo, starsHtml and MOVIE_RATING
in Modal.ts and replace the inline template injection for textual fields with
explicit DOM nodes whose textContent is set to the sanitized values.
| <button class="close-modal" id="closeModal"> | ||
| <img src=${modal_close} /> |
There was a problem hiding this comment.
닫기 버튼에 접근 가능한 이름이 없습니다.
현재 버튼 안에는 alt 없는 이미지뿐이라 보조기기에서는 용도를 알기 어렵습니다. 버튼 자체에 aria-label을 주고, 아이콘이 장식용이라면 이미지는 빈 alt로 처리해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/UI/Modal.ts` around lines 36 - 37, Add an accessible name to the
close button and mark the image as decorative: update the element with id
"closeModal" (class "close-modal") to include an appropriate aria-label (e.g.,
aria-label="Close modal") and make the nested <img> decorative by giving it an
empty alt attribute ("") so screen readers announce the button purpose rather
than the image.
| const star = target.closest(".star") as HTMLImageElement; | ||
|
|
||
| if (!star) { | ||
| return; | ||
| } | ||
|
|
||
| const id = Number(modal.dataset.id); | ||
| const rating = Number(star.dataset.id); | ||
|
|
||
| setRating(id, rating); | ||
| closeModal(modalBackground); | ||
| await controlModal(id); |
There was a problem hiding this comment.
평균 평점 아이콘도 별점 클릭으로 처리됩니다.
target.closest(".star")는 .rate 안의 장식용 별까지 잡기 때문에, 그 아이콘을 누르면 star.dataset.id가 없어 NaN이 저장됩니다. 클릭 대상을 실제 별점 영역으로만 좁히거나 data-id 유무를 먼저 확인해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.ts` around lines 126 - 137, target.closest(".star") can match
decorative stars inside .rate that lack data-id, producing NaN for rating;
update the click handling around target.closest(".star") so it only selects
stars with a data-id (e.g., use selector like '.star[data-id]' or check that
star.dataset.id exists before parsing), and if missing return early instead of
calling setRating/closeModal/controlModal; ensure you reference the same
modal/modalBackground flow and use the id/rating only when star.dataset.id is
defined.
| .detail-content { | ||
| max-height: 430px; | ||
| overflow-y: auto; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 15px; | ||
| font-family: "Montserrat"; | ||
| font-weight: 300; | ||
| font-style: Light; |
There was a problem hiding this comment.
여기서 stylelint가 깨집니다.
font-style: Light는 유효한 값이 아니고, font-family: "Montserrat"도 현재 규칙에 걸립니다. 지금 상태면 CI가 실패하거나 일부 선언이 브라우저에서 무시됩니다.
Also applies to: 166-167
🧰 Tools
🪛 Stylelint (17.7.0)
[error] 126-126: Expected no quotes around "Montserrat" (font-family-name-quotes)
(font-family-name-quotes)
[error] 128-128: Expected "Light" to be "light" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/styles/modal.css` around lines 120 - 128, The .detail-content CSS
contains invalid declarations: replace the invalid font-style: Light with a
valid font-style (e.g., normal/italic/oblique) and rely on font-weight: 300 for
light weight, and change font-family: "Montserrat" to include a generic fallback
(e.g., 'Montserrat', sans-serif) to satisfy stylelint; apply the same fixes to
the other occurrences referenced (lines 166-167) so CI/stylelint stops failing
and browsers don't ignore these rules.
| poster_path: string; | ||
| vote_average: number; | ||
| overview: string; | ||
| genres: [{ name: string }]; |
There was a problem hiding this comment.
genres 타입이 배열이 아닌 단일 요소 튜플로 정의되어 있습니다.
현재 [{ name: string }]는 정확히 1개의 요소만 가진 튜플 타입입니다. 하지만 TMDB API는 여러 장르를 반환하고, Modal.ts에서 .map()으로 여러 장르를 처리하며, 테스트에서도 2개의 장르를 기대합니다.
올바른 타입 정의를 위해 어떤 문법을 사용해야 할지 생각해 보시겠어요? 힌트: 가변 길이 배열을 표현하는 TypeScript 문법을 확인해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@types/types.ts` at line 20, The genres property is currently typed as a
single-element tuple `[{ name: string }]` causing runtime mismatches; change the
type of the `genres` property in types/types.ts (the `genres` field) from a
single-element tuple to a variable-length array type (e.g., use `{ name: string
}[]` or `Array<{ name: string }>`), so components like Modal.ts and tests that
expect multiple genres work correctly.
There was a problem hiding this comment.
안녕하세요 수야!
제가 질문만 하고 제 생각은 언급하지 않았네요..!
우선 제 생각을 먼저 말씀드리겠습니다!
- Modal의 getRating
위에서 짚은 레이어 역할 경계와 비슷한 맥락인데요. 영화 상세 데이터는 controller에서 가져와서 Modal에 넘겨주는데, 별점만 Modal이 직접 localStorage에 접근하고 있잖아요. 디움이 생각하는 "UI 컴포넌트가 어디까지 데이터에 접근해도 되는지"의 기준이 궁금합니다.
수정 전 코드에서 Modal이 직접 getRating 메서드를 호출해서 localStorage에 접근하는 방식이 과연 맞는지 의문이 생겨서 질문을 드렸습니다. 코드와는 상반된 의견이긴 하지만 저는 UI 컴포넌트는 UI라는 역할만 담당해야 한다고 생각하며 데이터에 접근하면 안된다고 생각합니다. 모든 데이터는 외부에서 주입받아 화면에 띄우는 역할만 해야한다고 생각합니다!
- renderHandlers 전역 인스턴스
현재 규모에서는 문제없어보입니다. renderHandlers의 역할이 UI를 컨트롤하는 거니까, UI 인스턴스를 renderHandlers가 직접 갖고 있는 건 자연스럽다고 생각해요.
디움의 생각이 궁금합니다. 어떤 부분 때문에 고민을 하셨는지 궁금해요.
- 전역 변수 상태 관리디움은 "논의해보고 싶다"고 했는데, 디움의 의견이 먼저 궁금해요! 지금 구조에서 불편한 점이 있었는지, 어떤 방향으로 개선하고 싶은지 공유해주면 더 구체적인 논의가 가능할 것 같아요.
3번과 4번을 질문한 이유는 각 모듈은 하나의 역할을 담당해야 하는데, 예를 들어 컨트롤러는 흐름 제어, 렌더는 각 클래스 렌더메서드 호출만을 담당해야 한다고 생각하기 때문입니다. 하지만 현재 각 클래스 인스턴스와 page 같은 변수를 전역으로 선언하고 사용하고 있는데, 이번 영화 리뷰 미션 같은 경우에 규모가 작아 전역으로 두어도 괜찮을 것 같지만, 규모가 크거나 구조가 복잡해지면 상태 관리 파일을 하나 만들어서 해당 파일 내부에서 전역으로 선언한 값들을 관리해야 한다가 제 생각입니다.
제가 여기서 의문이 드는 건 규모가 작은 경우 별도로 분리해서 관리하는 게 오버 엔지니어링인가? 입니다. 개인적으로 저는 규모가 작아도 오버 엔지니어링이 아니라고 생각합니다. 전역 변수를 사용하여 값 변경, 초기화는 쉽게 할 수 있었지만, 과연 별도의 파일로 분리하였을 때 값 변경을 하는 방식이 무엇인지 궁금해서 해당 질문을 하게 되었습니다!
변경 사항
작업 내용
main.ts의 비즈니스 로직을 controllerHandlers.ts로 이동했습니다.
무한 스크롤에서 마지막 페이지 이후 API를 호출하지 않도록 수정했습니다.
별점 선택 시 모달을 닫고 다시 조회하지 않고, 현재 열린 모달의 별점 영역만 즉시 갱신하도록 변경했습니다.
모달 닫기 시 .modal-background를 DOM에서 제거하도록 수정하고, 중복 id를 제거했습니다.
썸네일 이미지 로딩 중 placeholder가 보이도록 카드 단위 스켈레톤을 추가했습니다.
E2E 테스트를 구현 의존 방식에서 사용자 행동 기반으로 수정했습니다.
상세 변경
컨트롤러에 page, searchMovie, totalPage 상태를 모아 관리
controlSearchSubmit, controlScroll, controlModalClose 등 이벤트별 컨트롤러 함수 추가
별점 저장 후 updateModalRating()으로 UI만 갱신
ESC, X 버튼, 검색 결과 없음, 자세히 보기 버튼 시나리오 E2E 추가
scroll/keydown 직접 dispatch 하던 테스트를 cy.scrollTo("bottom"), cy.get("body").type("{esc}")로 변경
스켈레톤
현재 제 로직에서 스켈레톤을 호출하지만 보이지 않는 이유는 fetch로 데이터를 불러와 바로 렌더 메서드를 호출하는데, 렌더 메서드에 기존 UI를 지우는 로직이 존재하여 보이지 않는다고 생각합니다. 기존 스켈레톤은 데이터 요청만 커버했고, 실질적으로 오래 걸리는 이미지 로딩은 커버하지 못했습니다. 그래서 네트워크 환경에 따라 스켈레톤이 잠깐 보였다가 사라지거나, 아예 안 보였습니다.
강제로 시간을 지연시켜 스켈레톤이 보이는 것을 확인하였으나 강제로 지연시키는 방식은 좋지 않다고 생각하여 이미지 로딩에 시간이 오래 걸리는 점을 파악해서 MovieCard에 thumbnail-placeholder를 추가하여 API 응답 후 포스터 이미지가 실제로 보여질 때까지 스켈레톤을 보여주도록 하였습니다.
스켈레톤 UI가 보이긴 하지만 원래 제가 의도해서 구현했던 스켈레톤 클래스를 유의미하게 사용하진 못한 것 같습니다. 해당 부분에 대해서 스켈레톤 클래스를 유의미하게 사용할 수 있는 방식이 궁금합니다!
| // 무한 스크롤 | ||
| window.addEventListener("scroll", async () => { | ||
| if (window.innerHeight + window.scrollY >= document.body.scrollHeight - 1) { | ||
| page += 1; | ||
| await appendNextPageMovies(page, searchMovie); | ||
| } | ||
| }); | ||
|
|
2 participants
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
안녕하세요 수야! 우선 1단계에서 제가 설계한 구조를 유지하면서 2단계 미션을 진행하였습니다.
지금 구조에서 무한 스크롤, 모달 기능을 추가하면서 장단점이 각각 있었습니다.
장점 : 각 모듈마다 역할이 명확히 정해져있어 구조를 건드리지 않고 역할에 맞게 메서드, 클래스를 추가하면 되는 점
단점 : 더보기 버튼을 삭제하고 무한 스크롤을 추가하는 과정에서 moreButton을 사용하는 수많은 메서드를 일일이 찾아서 삭제한 점
더보기 버튼을 삭제하면서 더보기 기능이 모듈에 대한 의존성이 높다고 생각하였습니다.
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
2단계에서 많이 고민했던 문제는 다음과 같습니다.
영화 카드 클릭 시 해당 영화의 id 가져오기
MovieCard의 최상의 태그에 data-id 속성에 movie.id를 넣고, 클릭 이벤트에서 dataset.id를 읽어서 controlModal로 그 값을 넘겨서 해결하였습니다. 찾아본 결과 data-* 속성말고 getAttribute()를 사용하는 방식 또한 존재한다는 걸 배웠습니다. movie-id"123"을 불러오기 위해 getAttribute("movie-id")를 사용할 수 있습니다.
innterHTML을 사용하니 원래 그려져 있던 HTML을 통째로 지워버리는 문제
처음에는 Modal 클래스에서 정적 html인 class="container"인 div태그에 innerHTML을 사용하여 모달을 화면에 띄우도록 하였습니다. 하지만 innerHTML은 기존 HTML을 지우고 통째로 교체를 하기 때문에 이미 렌더링되어 있던 MovieList가 사라지는 문제가 발생했습니다. 그래서 기존 콘텐츠는 유지한 채 모달만 추가로 붙이기 위해 insertAdjacentHTML("beforeend", ...)를 사용했습니다. 이 방식으로 .container 내부의 마지막 위치에 모달만 삽입하므로, 기존 영화 목록은 유지되고 모달만 화면에 추가로 렌더링할 수 있었습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
z-index를 활용한 해결 방식입니다. 실제로 로고나 버튼이 눌리지 않는 문제가 있었고, 최종적으로는 z-index 조정으로 해결했지만, 이 방식이 근본적인 해결인지에 대해서는 고민이 남아 있습니다. 단순히 숫자를 맞춰 해결하는 것이 과연 좋은 방법인지 의문이 듭니다.
현재 Modal의 render 메서드에서 getRating을 호출해 별점에 따른 img 경로를 다르게 설정하고 있는데, Modal 같은 UI 컴포넌트가 어디까지 상태 로직을 가져가야 하는지 수야의 의견이 궁금합니다!
renderHandlers에서 Header, MainTitle, MovieList를 전역으로 생성해두고 재사용하는 구조가 현재는 단순해 보이지만, 이런 전역성에 가까운 UI 인스턴스 관리 방식이 이후 유지보수나 테스트 측면에서도 계속 유리한지 궁금합니다. 특히 화면이 복잡해지거나 초기화 시점이 늘어났을 때, 지금 구조가 책임을 명확하게 유지할 수 있는지에 대한 수야의 의견이 궁금합니다!
main.ts에서 페이지와 검색어를 전역 변수로 관리하고 있는데, 현재 구조에서 이 방식이 가장 단순하고 적절한지, 아니면 별도의 상태 관리 방식으로 분리하는 편이 더 나은지 논의해보고 싶습니다.
✅ 리뷰어 체크 포인트