-
Notifications
You must be signed in to change notification settings - Fork 8
[이준영_Frontend] 11주차 과제입니다. #2
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
base: main
Are you sure you want to change the base?
Conversation
JeongWon-CHO
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다 🙂
처음에는 React가 어색해서 힘들 수도 있지만, 하다 보면 React가 훨씬 더 편하다고 느끼실 거예요 !
앞으로도 파이팅입니다 !
src/App.ts
Outdated
| document.body.innerHTML = ` | ||
| ${Header.template()} | ||
| ${MovieSection.template()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
document.body.innerHTML로 전체 UI를 초기화하는 방식은 React스럽지 않은 것 같습니다 ..! 예를 들어,
function App() { return ( <> <Header /> <MovieSection /> </> ); }
이런 식으로 표현할 수 있을 거 같습니다 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
감사합니다. 리액트 마이그레이션 하면서 수정했습니다.
| import { PageTitle } from "./PageTitle"; | ||
|
|
||
| export const MovieList = { | ||
| movieContainer: document.getElementById('movie-list')!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOM 요소를 바로 참조하면 렌더링 타이밍에 따라 null이 될 수도 있어요 😅
렌더링 후 DOM 요소를 가져오도록 수정하면 좋을 거 같아요 !
| this.clearSkeleton(); | ||
| } | ||
| }, | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadMovies와 loadSkeletonUI에서 각각 요소를 생성하고 DOM에 추가하고 있는데, 이러한 방식은 DOM 업데이트를 여러 번 유발합니다 😊 배치 업데이트를 사용해 DOM 변경을 줄일 수 있습니다 !
| @media (max-width: 860px) { | ||
| #movie-container ul { | ||
| grid-template-columns: repeat(3, 1fr); | ||
| gap: 32px; | ||
| } | ||
| } | ||
|
|
||
| @media (max-width: 480px) { | ||
| header { | ||
| padding: 8px 16px; | ||
| } | ||
|
|
||
| #movie-container ul { | ||
| grid-template-columns: repeat(2, 1fr); | ||
| gap: 20px; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사이즈 기준을 다르게 준 이유는 무엇인가요 ??!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테블릿, 스마트폰에서 확인할 경우 반응형 웹페이지 적용을 위해서 기준을 다르게 주었습니다.
src/components/header/Search.ts
Outdated
| async searching() { | ||
| state.currentQuery = this.searchInput.value.trim(); | ||
|
|
||
| if (state.currentQuery) { | ||
| state.isSearching = true; | ||
| state.currentPage = 1; | ||
| MovieList.clearMovies(); | ||
| MovieList.loadSkeletonUI(8); | ||
|
|
||
| try { | ||
| const movies = await searchMoviesByName(state.currentQuery, state.currentPage); | ||
| MovieList.clearSkeleton(); | ||
|
|
||
| if (movies.length === 0) { | ||
| state.hasResults = false; | ||
| } else { | ||
| state.hasResults = true; | ||
| } | ||
|
|
||
| PageTitle.updatePageTitle(state.currentQuery, state.hasResults); | ||
|
|
||
| MovieList.loadMovies(movies); | ||
| Button.toggleLoadMoreButton(movies.length); | ||
| this.searchInput.value = ''; | ||
| } catch (error) { | ||
| handleError(error, '검색 중 오류 발생'); | ||
| MovieList.clearMovies(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
searching 메서드에 여러 기능이 들어가 있습니다 ! 역할을 분리해 보는 건 어떨까요 ?? 🧐
src/styles/default.css
Outdated
| html { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%와 vw의 차이점은 무엇일까요 ?? 🫠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%는 부모 요소의 크기를 기준으로 조절하며 vw는 브라우저 viewport 크기를 기준으로 조절합니다.
src/styles/default.css
Outdated
| /* * { | ||
| box-sizing: border-box; | ||
| } */ | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 주석은 삭제하시면 될 거 같습니다 !
그리고 개행도 컨벤션이라는 사실을 알고 계시나요 ?? 이에 대해서도 한 번 찾아보시면 좋을 거 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 알겠습니다.
No description provided.