[2단계 - 상세 정보 & UI/UX 개선하기] 비비빙 미션 제출합니다.#318
Open
binggwa wants to merge 32 commits intowoowacourse:binggwafrom
Open
[2단계 - 상세 정보 & UI/UX 개선하기] 비비빙 미션 제출합니다.#318binggwa wants to merge 32 commits intowoowacourse:binggwafrom
binggwa wants to merge 32 commits intowoowacourse:binggwafrom
Conversation
Summary by CodeRabbit릴리스 노트
Walkthrough이 변경 사항은 영화 리뷰 애플리케이션의 단계 2 기능을 구현합니다. 모달 기반 영화 상세 정보 보기, 사용자 별점 시스템(localStorage 기반 영속성), 무한 스크롤 기능(기존 "더보기" 버튼 대체), 반응형 디자인 개선을 추가합니다. 코드베이스는 모달 관리 모듈, 별점 저장소, 재구성된 영화 목록 앱으로 리팩터링되었으며, 렌더링 로직이 분리되었고 E2E 테스트 커버리지가 추가되었습니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api.ts (1)
60-62:⚠️ Potential issue | 🟠 MajorTMDB API 파라미터 형식 오류
TMDB API 문서 기준으로:
language: ISO 639-1 언어 코드에 ISO 3166-1 국가 코드를 결합한 형식 (예:ko-KR,en-US,pt-BR)region: ISO 3166-1 알파-2 국가 코드 대문자만 사용 (예:KR,US,DE)현재 코드는 이 두 파라미터가 바뀌어 있습니다:
region='ko-KR'→'KR'로 수정 필요language='ko'→'ko-KR'로 수정 필요60-62번과 90-91번 두 위치 모두 같은 방식으로 수정해야 합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api.ts` around lines 60 - 62, The TMDB request parameters are swapped: update the two occurrences where you call url.searchParams.append('region', 'ko-KR') and url.searchParams.append('language', 'ko') so that region uses the ISO 3166-1 alpha-2 code ('KR') and language uses the locale with country ('ko-KR'); locate both places that append these params (the blocks containing url.searchParams.append('page', ...), url.searchParams.append('region', ...), url.searchParams.append('language', ...)) and swap the argument values accordingly.
🧹 Nitpick comments (9)
.github/pull_request_template2.md (1)
18-23: 논의 내용 섹션에 플레이스홀더 추가를 고려해보세요.리뷰 요청 섹션(1번, 2번 질문)이 비어 있습니다. 사용자가 어떤 내용을 작성해야 할지 힌트를 제공하는 플레이스홀더 텍스트를 추가하면 템플릿 활용도가 높아질 수 있습니다.
💡 제안하는 개선 사항
#### 1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점 + +(여기에 작성해주세요) #### 2) 이번 리뷰를 통해 논의하고 싶은 부분 + +(여기에 작성해주세요)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/pull_request_template2.md around lines 18 - 23, Add placeholder hint text under the two review questions so contributors know what to write: update the "#### 1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점" and "#### 2) 이번 리뷰를 통해 논의하고 싶은 부분" sections to include brief example prompts (e.g., "요약: ...", "핵심 결정: ...", "검토 요청 포인트: ...") or short placeholder sentences to guide input; keep the placeholders concise and neutral so they appear only as examples in the template.cypress/e2e/spec.cy.ts (1)
44-53: 주석 처리된 테스트는 제거하거나 새 시나리오로 교체해 주세요.Line 44-53은 현재 실행되지 않아 회귀 검증에 도움이 되지 않습니다. 무한 스크롤 기준의 활성 테스트로 대체하거나 불필요 코드라면 삭제하는 편이 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/spec.cy.ts` around lines 44 - 53, The commented Cypress test block for "더보기 버튼을 누르면..." should be removed or replaced with an active test: either delete the entire commented it(...) block, or implement a new active scenario that verifies infinite scrolling or the "load more" behavior using the existing stubs/aliases (`@getPopularMovies`, `@getNextPopularMovies`) and selectors (`#more-page-button`, .thumbnail-list > li); ensure the new test visits '/', waits for the first page, triggers the UX (click `#more-page-button` or simulate scroll), waits for the next request alias, and asserts the expected item count instead of leaving the test commented out.search.html (1)
18-18: 헤더 인라인 스타일은 CSS 클래스로 분리하는 편이 좋습니다.Line 18의
style속성은 반응형 수정 시 누락 위험이 있어, 스타일시트로 이동하면 유지보수가 쉬워집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@search.html` at line 18, 현재 header 엘리먼트의 인라인 style 속성("position: relative; width: 100%;")을 CSS 클래스로 분리하세요: search.html의 <header> 태그에서 인라인 style을 제거하고 예: class="site-header" 같은 클래스를 추가한 뒤 공용 CSS(또는 해당 페이지의 스타일시트)에 .site-header { position: relative; width: 100%; } 를 정의하고 미디어 쿼리가 필요한 반응형 규칙도 동일한 CSS 파일에 추가하여 유지보수와 반응형 수정 시 누락을 방지하세요.package.json (1)
10-13:predeploy와deploy의 빌드 중복을 정리하는 것을 권장합니다.Line 10과 Line 13에서 빌드가 중복 실행되고 옵션도 달라 배포 흐름이 불필요하게 복잡해집니다. 빌드는 한 경로로 통일하고 deploy는 게시 역할만 담당하도록 분리해 보세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 10 - 13, Unify the build step by moving all build options into the predeploy script and make deploy only handle publishing: update the "predeploy" script to perform the Vite build with the final desired flags (base and minify) and remove the build invocation from the "deploy" script so "deploy" only runs the publishing tool (npx gh-pages -d dist); modify the "predeploy" and "deploy" npm scripts accordingly to avoid duplicated builds and ensure a single authoritative build command.styles/skeleton.css (1)
2-12: 모션 감소 설정도 같이 넣어주세요.현재 shimmer 애니메이션이 항상 무한 반복이라, 이 로딩 UI를 정적으로 보여줘야 하는 사용자도 끌 수단이 없습니다.
prefers-reduced-motion분기를 같이 두면 접근성 리스크를 줄일 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@styles/skeleton.css` around lines 2 - 12, Update the .skeleton rules and `@keyframes` usage to respect users' prefers-reduced-motion setting: add a media query for (prefers-reduced-motion: reduce) that targets the .skeleton selector and disables the shimmer by setting animation: none and fixing a static background-position (matching either 0% or the resting position used in `@keyframes` skeleton-loading); keep the existing animation only outside that media query. Reference the .skeleton class and `@keyframes` skeleton-loading when locating the code to change.src/movieListApp.ts (1)
48-55: 페이지 로드 진입점이 셋으로 갈라져 있어 상태 드리프트가 생기기 쉽습니다.초기 로드, 인터섹션 콜백, 버튼 재시도에서
isFetching토글과loadMovies()호출 규칙이 반복됩니다. 다음에 로딩/재시도 규칙을 바꾸면 한 군데만 수정되고 나머지가 어긋나기 쉬워서, 이 부분은 공통 helper로 모아두는 편이 유지보수에 유리합니다. As per coding guidelines, "Promote modular thinking—breaking problems into reusable components."Also applies to: 66-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/movieListApp.ts` around lines 48 - 55, The repeated pattern toggling isFetching and calling loadMovies (seen in handleIntersect, the initial load entrypoint, and the retry button handler) should be extracted into a single helper to avoid state drift; create a function (e.g., performLoadMovies or safeLoadMovies) that checks isError and isFetching, sets isFetching=true, calls await loadMovies(), and finally sets isFetching=false (with try/finally), then replace the inline logic in handleIntersect, the initial startup code, and the retry handler to call that helper instead of duplicating the toggle logic.cypress/e2e/movie.cy.ts (3)
58-65: 테스트 일관성을 위해cy.wait('@getMovieDetail')추가를 고려하세요.첫 번째 모달 테스트(Line 44)에서는
cy.wait('@getMovieDetail')을 사용하지만, 이 테스트에서는 누락되어 있습니다. 모달이 열린 직후 배경 클릭을 하면 API 응답 전에 동작이 실행될 수 있어 테스트 결과가 불안정해질 수 있습니다.♻️ 일관성 있는 테스트를 위한 수정 제안
it('모달 배경을 클릭해도 모달이 닫힌다.', () => { cy.get('.thumbnail-list .movie-item').first().click(); + cy.wait('@getMovieDetail'); + // 배경 영역 클릭 cy.get('#modalBackground').click('topLeft', { force: true }); cy.get('#modalBackground').should('not.have.class', 'active'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/movie.cy.ts` around lines 58 - 65, The test clicking a movie thumbnail then the modal background lacks synchronization and should wait for the movie detail API; after triggering the modal open via cy.get('.thumbnail-list .movie-item').first().click() add a cy.wait('@getMovieDetail') before interacting with '#modalBackground' so the modal is fully loaded, then proceed with the background click and the existing assertion that '#modalBackground' does not have the 'active' class.
80-116: 별점 테스트 로직은 좋지만, API 응답 대기가 누락되어 있습니다.테스트 시나리오와 검증 로직이 잘 구성되어 있습니다. 다만 Line 86과 Line 104에서 영화 아이템 클릭 후
cy.wait('@getMovieDetail')이 없어서, 모달 콘텐츠가 완전히 렌더링되기 전에 별점 클릭이나 검증이 실행될 수 있습니다.♻️ API 응답 대기 추가 제안
// 모달 열기 cy.get('.thumbnail-list .movie-item').first().click(); + cy.wait('@getMovieDetail'); // 8점 클릭 cy.get(`.rate-star-img[data-score="${TARGET_SCORE}"]`).click();// 동일한 영화의 모달을 다시 열기 cy.get('.thumbnail-list .movie-item').first().click(); + cy.wait('@getMovieDetail'); // LocalStorage에서 데이터를 잘 가져와서 이전 점수가 그대로 표시되는지 확인🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/movie.cy.ts` around lines 80 - 116, After opening the movie modal via '.thumbnail-list .movie-item'.first().click(), wait for the movie detail API mock by adding cy.wait('@getMovieDetail') before interacting with `.rate-star-img[data-score="${TARGET_SCORE}"]`; likewise after reopening the modal (the second click) add cy.wait('@getMovieDetail') before asserting on '#ratingDescription' and checking `.rate-star-img[data-score="2"]` so the modal content is fully rendered before clicks/assertions.
68-77: ESC 키 테스트에도 동일하게cy.wait('@getMovieDetail')추가를 고려하세요.다른 모달 테스트들과의 일관성을 위해 API 응답 대기를 추가하면 테스트 안정성이 향상됩니다.
♻️ 수정 제안
it('ESC 키를 누르면 모달이 닫힌다.', () => { cy.get('.thumbnail-list .movie-item').first().click(); + cy.wait('@getMovieDetail'); cy.get('#modalBackground').should('have.class', 'active');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypress/e2e/movie.cy.ts` around lines 68 - 77, Add an explicit wait for the movie detail API alias after opening the modal: in the "ESC 키를 누르면 모달이 닫힌다." test, after clicking the first '.thumbnail-list .movie-item' and before asserting the modal is active, call cy.wait('@getMovieDetail') to ensure the API response has completed (using the same alias '@getMovieDetail' used in other modal tests) so the modal state assertions are stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/pull_request_template2.md:
- Line 16: 현재 "(해당하는 경우) 배포 링크 기입: \***\*\_\_\*\***"의 플레이스홀더(\***\*\_\_\*\***)는
이스케이프된 마크다운 문자로 인해 잘못 렌더링될 수 있으니, 해당 플레이스홀더 문자열(\***\*\_\_\*\***)을 더 명확한 텍스트
플레이스홀더로 교체하세요 — 예: "(해당하는 경우) 배포 링크 기입: <배포 URL>" 또는 "(해당하는 경우) 배포 링크 기입:
https://your-deployment.example"처럼 백슬래시나 별표/밑줄 이스케이프를 제거한 단순한 형식으로 수정해 문서 렌더링과
사용자가 채우기 쉬운 형태로 만드세요.
In `@index.html`:
- Around line 58-67: The modal markup lacks dialog semantics and focus
management; update the elements with appropriate ARIA and focus handling: give
the modal container (id="modalContainer" or the element with class "modal")
role="dialog" and aria-modal="true", add an accessible label via aria-labelledby
referencing a visible heading inside the modal, ensure the close button
(id="closeModal") has an accessible name (text or aria-label), implement
keyboard handlers to trap focus within the modal (on open move focus to the
first focusable element in modalContainer and on Close/ESC restore focus to the
element that opened the modal), and prevent background content from being
focusable while modalBackground is active.
In `@search.html`:
- Around line 46-55: The modal markup (elements with ids modalBackground, modal,
closeModal, modalContainer) lacks accessibility attributes and focus management;
update the modal container to include role="dialog" and aria-modal="true" and
either aria-labelledby pointing to a visible heading inside modalContainer or an
aria-label on modalContainer, ensure the close button has an accessible name,
add tabindex="-1" on modalContainer so it can receive programmatic focus, and
implement focus-trap behavior when opening (move focus into modalContainer or
the first focusable element) and restore focus to the previously focused element
when closing (also handle Escape key to close and ensure closeModal is
focusable). Ensure these changes are applied around the elements with ids
modalBackground, modal, closeModal, and modalContainer.
In `@src/modal.ts`:
- Around line 11-25: openModal can suffer from race conditions when multiple
calls overlap; add a guard token so only the latest request updates the UI:
introduce a module-scoped "latestRequestId" (or incrementing requestCounter) and
capture its value at the start of openModal, then after awaiting
fetchMovieDetail (and any subsequent awaits like reviewStorage.getRating)
compare the captured id to the current latestRequestId and abort updating
modalState/modalView if they differ; alternatively use an AbortController passed
to fetchMovieDetail and check for abort before rendering. Ensure you reference
modalState, modalView, fetchMovieDetail, and reviewStorage in the check so only
the last-opened movieId is rendered.
- Around line 46-55: handleStarClick updates modalState and UI before awaiting
reviewStorage.saveRating, so if save fails the UI and state stay inconsistent;
change the flow in handleStarClick to attempt saving first (await
reviewStorage.saveRating(modalState.movieId, clickedScore)) and only on success
update modalState.savedRating and call modalView.updateStarsUI(clickedScore),
and on failure revert UI/state or keep previous score and surface an error
message (e.g., via modalView.showError) and optionally allow retry; ensure you
reference handleStarClick, modalState.savedRating, modalView.updateStarsUI, and
reviewStorage.saveRating when making this change.
In `@src/modalView.ts`:
- Around line 26-33: showModalSkeleton currently only injects a text message and
does not render the skeleton placeholders defined in skeleton.css; update the
function (referencing showModalSkeleton and $modalContainer) to include
placeholder markup using the .skeleton wrapper and child elements
.skeleton-thumbnail and one or more .skeleton-text blocks inside the injected
template so the skeleton styles are actually applied when loading.
- Around line 57-64: The modal currently writes untrusted API fields
(data.title, genres, data.overview, posterUrl) directly into innerHTML via the
template that calls createPosterHTML, createMovieHeaderHTML, createMyRatingHTML
and createPlotHTML; replace this with a safe construction: do not concatenate
raw strings into $modalContainer.innerHTML—either escape all interpolated values
using a utility like escapeHTML or sanitize the assembled HTML with a vetted
sanitizer (e.g., DOMPurify) before assigning, or better yet build the modal DOM
using createElement/setAttribute/textContent and call appendChild for
poster/title/genres/overview to ensure values are set as text nodes; update the
code paths in createPosterHTML, createMovieHeaderHTML and createPlotHTML (or
their callers) to accept sanitized/plain-text inputs instead of embedding raw
API strings.
In `@src/view.ts`:
- Around line 55-66: User input (query) is interpolated into the template in
renderEmptyPage and then assigned via $target.innerHTML in renderEmptyState,
creating an XSS risk; fix by preventing raw HTML insertion—either implement an
escapeHtml utility and use it to escape query inside renderEmptyPage before
interpolation, or stop using innerHTML and instead build the DOM nodes in
renderEmptyState (createElement for the wrapper, img element, and set the
message node's textContent to the raw query) so the browser treats it as text;
update references to renderEmptyPage/renderEmptyState and the $target.innerHTML
usage accordingly.
In `@styles/main.css`:
- Around line 79-92: Remove the extra blank line(s) inside the
.pretty-search-bar rule so there are no empty lines between declarations (e.g.,
between "justify-content: space-between;" and "width: 90%;" and between
"max-width: 480px;" and "height: 48px;"); edit the .pretty-search-bar block to
have contiguous declaration lines to satisfy the declaration-empty-line-before
stylelint rule.
In `@styles/modal.css`:
- Around line 190-199: Remove the stray blank line inside the .modal rule that
violates the stylelint declaration-empty-line-before rule (between max-height:
50vh; and transform: translateY(100%);) — keep all declarations contiguous by
deleting the empty line so the .modal block has no empty line before the
transform/animation declarations.
- Around line 271-273: Rename the `@keyframes` identifier from slideUp to
kebab-case slide-up and update any CSS properties that reference it (e.g., the
animation or animation-name declarations that currently use slideUp) so they use
slide-up instead; search for all occurrences of "slideUp" in the stylesheet
(notably the rule that sets animation on the modal) and replace them with
"slide-up" to satisfy the keyframes-name-pattern rule.
In `@styles/thumbnail.css`:
- Around line 25-30: The mobile breakpoint for .thumbnail-list is too narrow
(max-width: 360px) so devices like 375px/390px still render three cramped
columns; update the media query (the `@media` rule around .thumbnail-list) to a
wider max-width (e.g., 414px or 420px) so grid-template-columns: repeat(1, 1fr)
takes effect for common mobile widths and keep the existing gap/padding values
or adjust them if needed to preserve spacing.
---
Outside diff comments:
In `@src/api.ts`:
- Around line 60-62: The TMDB request parameters are swapped: update the two
occurrences where you call url.searchParams.append('region', 'ko-KR') and
url.searchParams.append('language', 'ko') so that region uses the ISO 3166-1
alpha-2 code ('KR') and language uses the locale with country ('ko-KR'); locate
both places that append these params (the blocks containing
url.searchParams.append('page', ...), url.searchParams.append('region', ...),
url.searchParams.append('language', ...)) and swap the argument values
accordingly.
---
Nitpick comments:
In @.github/pull_request_template2.md:
- Around line 18-23: Add placeholder hint text under the two review questions so
contributors know what to write: update the "#### 1) 이번 단계에서 가장 많이 고민했던 문제와 해결
과정에서 배운 점" and "#### 2) 이번 리뷰를 통해 논의하고 싶은 부분" sections to include brief example
prompts (e.g., "요약: ...", "핵심 결정: ...", "검토 요청 포인트: ...") or short placeholder
sentences to guide input; keep the placeholders concise and neutral so they
appear only as examples in the template.
In `@cypress/e2e/movie.cy.ts`:
- Around line 58-65: The test clicking a movie thumbnail then the modal
background lacks synchronization and should wait for the movie detail API; after
triggering the modal open via cy.get('.thumbnail-list
.movie-item').first().click() add a cy.wait('@getMovieDetail') before
interacting with '#modalBackground' so the modal is fully loaded, then proceed
with the background click and the existing assertion that '#modalBackground'
does not have the 'active' class.
- Around line 80-116: After opening the movie modal via '.thumbnail-list
.movie-item'.first().click(), wait for the movie detail API mock by adding
cy.wait('@getMovieDetail') before interacting with
`.rate-star-img[data-score="${TARGET_SCORE}"]`; likewise after reopening the
modal (the second click) add cy.wait('@getMovieDetail') before asserting on
'#ratingDescription' and checking `.rate-star-img[data-score="2"]` so the modal
content is fully rendered before clicks/assertions.
- Around line 68-77: Add an explicit wait for the movie detail API alias after
opening the modal: in the "ESC 키를 누르면 모달이 닫힌다." test, after clicking the first
'.thumbnail-list .movie-item' and before asserting the modal is active, call
cy.wait('@getMovieDetail') to ensure the API response has completed (using the
same alias '@getMovieDetail' used in other modal tests) so the modal state
assertions are stable.
In `@cypress/e2e/spec.cy.ts`:
- Around line 44-53: The commented Cypress test block for "더보기 버튼을 누르면..."
should be removed or replaced with an active test: either delete the entire
commented it(...) block, or implement a new active scenario that verifies
infinite scrolling or the "load more" behavior using the existing stubs/aliases
(`@getPopularMovies`, `@getNextPopularMovies`) and selectors (`#more-page-button`,
.thumbnail-list > li); ensure the new test visits '/', waits for the first page,
triggers the UX (click `#more-page-button` or simulate scroll), waits for the next
request alias, and asserts the expected item count instead of leaving the test
commented out.
In `@package.json`:
- Around line 10-13: Unify the build step by moving all build options into the
predeploy script and make deploy only handle publishing: update the "predeploy"
script to perform the Vite build with the final desired flags (base and minify)
and remove the build invocation from the "deploy" script so "deploy" only runs
the publishing tool (npx gh-pages -d dist); modify the "predeploy" and "deploy"
npm scripts accordingly to avoid duplicated builds and ensure a single
authoritative build command.
In `@search.html`:
- Line 18: 현재 header 엘리먼트의 인라인 style 속성("position: relative; width: 100%;")을 CSS
클래스로 분리하세요: search.html의 <header> 태그에서 인라인 style을 제거하고 예: class="site-header" 같은
클래스를 추가한 뒤 공용 CSS(또는 해당 페이지의 스타일시트)에 .site-header { position: relative; width:
100%; } 를 정의하고 미디어 쿼리가 필요한 반응형 규칙도 동일한 CSS 파일에 추가하여 유지보수와 반응형 수정 시 누락을 방지하세요.
In `@src/movieListApp.ts`:
- Around line 48-55: The repeated pattern toggling isFetching and calling
loadMovies (seen in handleIntersect, the initial load entrypoint, and the retry
button handler) should be extracted into a single helper to avoid state drift;
create a function (e.g., performLoadMovies or safeLoadMovies) that checks
isError and isFetching, sets isFetching=true, calls await loadMovies(), and
finally sets isFetching=false (with try/finally), then replace the inline logic
in handleIntersect, the initial startup code, and the retry handler to call that
helper instead of duplicating the toggle logic.
In `@styles/skeleton.css`:
- Around line 2-12: Update the .skeleton rules and `@keyframes` usage to respect
users' prefers-reduced-motion setting: add a media query for
(prefers-reduced-motion: reduce) that targets the .skeleton selector and
disables the shimmer by setting animation: none and fixing a static
background-position (matching either 0% or the resting position used in
`@keyframes` skeleton-loading); keep the existing animation only outside that
media query. Reference the .skeleton class and `@keyframes` skeleton-loading when
locating the code to change.
🪄 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: a387882d-08c9-4fb4-bc72-aaf805569846
⛔ Files ignored due to path filters (6)
images/logo.pngis excluded by!**/*.pngimages/modal_button_close.pngis excluded by!**/*.pngimages/star_empty.pngis excluded by!**/*.pngimages/star_filled.pngis excluded by!**/*.pngimages/woowacourse_logo.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
.github/pull_request_template2.mdREADME2.mdcypress/e2e/movie.cy.tscypress/e2e/spec.cy.tscypress/fixtures/movieDetail.jsoncypress/fixtures/popularMovies.jsonindex.htmlpackage.jsonsearch.htmlsrc/api.tssrc/main.tssrc/modal.tssrc/modalView.tssrc/movieController.tssrc/movieListApp.tssrc/render.tssrc/reviewStorage.tssrc/search.tssrc/view.tsstyles/main.cssstyles/modal.cssstyles/skeleton.cssstyles/thumbnail.css
💤 Files with no reviewable changes (2)
- src/movieController.ts
- src/render.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 미션 소개
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
안녕하세요 우코 ㅎㅎ 1단계 리뷰 덕분에 추상화 레벨과 URLSearchParams에 대해 배우고, 실행흐름에 관한 고민거리를 던져주셔서 많은 도움이 되었습니다!! 2단계 리뷰도 잘 부탁드립니다!🙏🙏 이번에는 저번에 못했던 리팩토링을 좀 진행해보았는데요, 기존 render가 담당하고 있던 fetch 받아와서 render하는 renderfetch와 같은 무거운 함수를 movieListApp (기존 movieController입니다) 이 메인흐름을 담당하는 파일이었기에 여기로 옮겨왔습니다. 그리고 render와 view에서 겹쳐져 있던 중복코드를 view 하나로 통합했습니다. step2도 기본기능 구현을 먼저 하고 PR 제출 전에 가능한만큼 리팩토링을 하고 있었습니다. 현재 modalView와 modal의 경우 리팩토링을 잘 진행한 것 같습니다만 기존 renderfetch를 movieListApp으로 옮기면서 이번엔 이파일이 너무 비대해져 리팩토링 진행도중 마감시간이 다가와 제출드립니다 ㅠㅠ 리팩토링이 잘 되었는지 중점적으로 봐주시면 감사드리겠습니다!!
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
🚨[많이 고민했던 문제]
이번 단계에서 가장 많이 고민했던 문제는 로컬과 웹을 갈아끼울 수 있도록 설계하는 부분이었습니다. 얼마전에 SOLID 원정대를 진행하면서 익힌 의존성 주입 원칙을 사용해 일단 reviewStorage 껍데기를 추상화하여 만들어놓고, 그걸 구체화한 class를 만들어 객체로 주입한다면, 만들어둔 코드를 주입하는 부분 한줄만 바꾸면 언제든 원하는 스토리지(local or API)로 변경할 수 있게 설계할 수 있다고 생각했습니다.
🌱 [배운점]
리팩토링 책과 예시 코드만 보고서는 SOLID 원칙의 유용함에 대해 크게 체감하지 못했는데, 이번 미션을 통해 실제로 DIP를 어떻게 사용하는지 그 실제 코드 활용법과 함께 그 강력함을 조금 느낄 수 있지 않았나 싶습니다. 더불어 DIP를 의식하면서 가능하면 SRP도 지키는 코드를 짜고 싶었는데, 단일책임원칙이 잘 지켜졌는지는 확신하지 못하겠네요.. 명확한 요구사항에 대한 해결방법으로는 사용했지만 제 전체 코드에 원칙을 체화시키는 건 정말 어려운 일임을 깨달은 것 같습니다. 혹시 우코가 보시기에는 DIP를 활용한 이런 해결방법이 방향에 괜찮아 보이시나요?
2) 이번 리뷰를 통해 논의하고 싶은 부분
🌪️ [논의하고 싶은 부분]
처음에는 단순하게 이벤트리스너를 달아서 스크롤이 화면 맨 아래에 도달하면 더보기버튼 클릭 시 로직을 그대로 가져다 쓰려고 생각했었는데, 이벤트리스너에 화면 맨 아래에 도달했음을 명시할 수 있는 기준을 찾지 못했습니다.. 또, 스크롤 이벤트를 다는 경우 스크롤을 계속할 때마다 이벤트가 발생하여 이벤트가 너무 많이 발생한다는 이야기를 보았습니다. 이 때문에 intersectionObserver라는 옵저버를 도입해서 더보기 버튼이 보이면 loadMovies하는 방식으로 진행했습니다. 이 때 옵저버를 0픽셀에 딱 반응하도록 만들면 화면 맨 아래에서 더보기 버튼이 순식간에 나왔다 사라지는 등 경험이 좋지 않아 보여 200픽셀 위에서 걸러내는 것으로 변경하여 데이터가 페치되는동안 자연스럽게 더보기 버튼을 확인하지 않고 스크롤할 수 있도록 만들었습니다. 혹시 현업에선 이런 무한 스크롤 요구사항은 어떤 식으로 처리하는지 궁금합니다.
여기서 또 문제가 하나 발생했었는데요, 위처럼 200픽셀 위에서 걸러내는 경우 스크롤을 너무 빨리 내리면 로딩중에 더보기버튼이 숨겨져있어서 옵저버가 더보기버튼을 200픽셀 위에서 잡아내지 못해 무한스크롤이 안되고 더보기버튼이 출력되는 문제가 있었습니다. 이 때문에 온전히 더보기버튼을 무한 스크롤로 대체하려던 초기 구상하고는 다르게 더보기버튼을 그대로 남겨둬서 화면 맨 아래에서도 목록이 더 나오지 않으면 (옵저버가 200px 트리거를 놓쳤으면) 다시 클릭할 수 있게 남겨두는 방안으로 진행했는데요, 이런 경우 무한 스크롤과 더보기버튼 방식의 혼용이 되는 것으로 보입니다. 해당 구조가 괜찮은 방향일까요? 혹시 더 스마트한 방법이 있었는지, 어떤 식으로 무한 스크롤을 짜야 자연스러웠을지 여쭤보고 싶습니다.
또 다른 문제로, 첫번째 영화목록을 불러오고 인터넷이 없는 상태에서 스크롤을 내렸을 경우, alert가 뜨고 다시 옵저버가 바로 버튼을 관찰해버려 실행시키는 문제가 생겼습니다. 이를 해결하기 위해 isError 오류 상태 변수를 추가해서, error가 한번뜨면 isError를 true로 변환하고, isError가 true인 동안에는 옵저버가 발동하지 않도록 했습니다. 사용자가 더보기버튼을 누르도록 유도하고 에러메시지에 더보기 유도문구를 추가한 다음, 더보기버튼을 클릭할 시 isError를 다시 false로 바꾸도록 로직을 만들었습니다. 그런데, 이런 방향이 괜찮았을까요? 결국 더보기버튼과 혼용되는 구조이기에 이런 방식이 가능했지만, 혹시 무한 스크롤만 있을 때에는 이런 것을 막는 방법이 따로 있었을까요?
에러가 났을 때, 어떤 메세지를 보여주고, 어떤 화면을 보여지게 하면 좋을지를 디자이너 분들께 여쭤본다고 해주셨는데, 혹시 시간이 허락한다면 보통 에러 노출 처리 이외에 실제로 에러화면 관련된 작업이 할게 많이 있나요? 시간이 충분하다면 어떤 에러 처리 관련 흐름을 어떻게 처리하시는지 궁금합니다!
반응형을 만들면서 css에 화면의 width기준으로 @media 태그를 달아 새로운 스타일을 만들어서 바꾸는 방향으로 만들었는데, 혹시 디자이너 분들께 받는 파일의 경우 화면 크기의 기준을 보통 어떻게, 얼마나 상세하게 세우는지 궁금합니다. 현재는 모바일 디자인 기준이 Viewport 360 픽셀이 주로 사용된다하여 모바일 스위칭 기준은 360px로 설정하고 태블릿은 1024px로 설정했습니다만, 반응형의 구현 정도를 어디까지 고려하는지 궁금했습니다. 또, 현재 화면 가로 width가 200 아래로 떨어지면 화면이 많이 깨지는 경향이 있습니다. 현재 회사가 모바일만 신경쓰신다고 하셨는데 디자이너 분들께서 아주 작은 화면은 어디까지 고려하실지 궁금합니다.
현재 내 별점 매기는 로직에서 innerHTML을 사용해서 내부를 바로바로 교체하는 식으로 진행하고 있는데, 이번 미션의 요구사항 중 하나로 리액트스러운 사용방법을 피하라는 내용이 있었던 것으로 기억합니다. 이 때문에 step1에서 영화목록의 경우 insertAdjacentHTML을 사용해서 뒤에 붙이는 식으로 사용했는데, 모달 창의 경우 요소를 붙이고 삭제하는 과정보다 innerHTML로 통째로 교체하는 방안이 더 직관적으로 다가와 요구사항을 준수하지 못했습니다. 우코가 생각하시는 리액트스러움과 저의 현재 방법이 괜찮았는지? 혹시 다른 방법은 없었을지, 우코는 이런 경우 어떤 방식으로 진행했었을까 여쭤보고 싶습니다.
✅ 리뷰어 체크 포인트