-
Notifications
You must be signed in to change notification settings - Fork 129
[2단계 - 상세 정보 & UI/UX 개선하기] 앵버(최승수) 미션 제출합니다. #255
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
Conversation
Co-authored-by: Seungsoo Choi <[email protected]>
Co-authored-by: Seungsoo Choi <[email protected]>
Co-authored-by: Seungsoo Choi <[email protected]>
|
궁금증) 팝업이 노출되면 뒤에 스크롤이 되는데 의도한걸까요? |
|
전체적으로 js 로 하셨는데, ts 를 안쓰신 이유가 있을까요? |
-> 지금처럼 단순한 과제에서는 도메인에서 직접 localStorage.getItem/setItem을 쓰는 방식도 충분히 괜찮다고 생각합니다. 하지만 정말 분리할 순간이 온다면, 저희는 be에서 사용하는 db의 도움을 받을수 없으니 class를 사용해 주입하는 방식으로 할거같아요 예를 들어서 별점 저장용 RatingStorage 클래스를 만들어서 도메인에서는 이렇게 의존성 주입해서 사용할거 같아요 |
-> 흠 서로 장단점이 분명해서 어렵네요.. 저희 팀에서는 하이브리드 방법을 사용하는데, API 응답을 받으면 텍스트와 플레이스홀더 이미지로 먼저 보여주면서 이미지가 로드되면 점차 바뀌는 방식을 사용하고 있습니다. |
JeongBin0227
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.
안녕하세요 앵버!
2단계도 잘 반영해주셔서 감사해요! 전체적으로 코드는 좋은데, 실제화면을 보니 버그가 좀 있는거 같아서 코멘트에 같이 정리해봤는데, 버그 먼저 고치고, 코멘트를 반영해주시면 될거같아요!
2단계 하느라 고생많으셨습니다~ 💯
| export function getDetailParam() { | ||
| return { | ||
| language: "ko-KR", | ||
| }; | ||
| } |
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.
아래와 같이 함수로 분리하지 않고 사용할 수도 있지만 함수명을 통해 값이 어떤 역할을 하는지 명시적으로 표현할 수 있고 다른 getSearchParam, getPopularParam 함수와 통일성을 유지할 수 있기 때문에 함수로 분리하여 작성했습니다.
const movieData = await movieService.fetchMovies(`/movie/${movieId}`, {
language: "ko-KR",
});| vote_average, | ||
| }); | ||
|
|
||
| $movieItem.addEventListener("click", (event) => onClick(event)); |
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.
👍
| }); | ||
|
|
||
| star.addEventListener("click", () => { | ||
| const rating = (index + 1) * 2; |
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.
이렇게 비지니스 로직을 짜게되면 나중에 유지보수 하기 힘들어 질거 같아요!
현재는 2점 단위로 별점을 계산하고 있는데, 혹시 추후 별 개수나 점수 단위를 바꿔야 할 경우를 고려해서 STAR_UNIT과 같이 상수화해서 관리하면 유지보수가 쉬워질 것 같아요~
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.
유지보수와 가독성을 위해서 상수화하는 게 맞다고 생각합니다.
types/movieType.ts
Outdated
|
|
||
| export interface MovieDetailInfo extends MovieInfo { | ||
| release_date: string; | ||
| genres: movieGenre[]; |
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.
여기도 genres 보단, movieGenres 가 어떨까요?
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.
실제 API에서는 영화 장르 정보를 담은 키로 "genres"를 사용하고 있는데 제가 이를 "movieGenres"와 같이 사용하고 싶다면 API에서 받은 데이터의 키 이름을 내부 데이터 모델에 맞게 변환하는 로직이 필요할 것 같습니다.
문제는 popularMovie, searchMovie, detailMovie API를 공통으로 사용하다 보니 각 API의 반환 데이터 구조가 다르다는 점인데
API의 반환 구조가 달라 모든 API 응답에 대해 동일한 변환 로직을 적용하기 어려웠습니다.
이 문제를 저는 AI 도움을 받아 adaptMovieData 함수로 해결했는데 현재는 해당 코드를 전부 이해하기 어려워 좀 더 타입스크립트에 대한 공부가 필요할 것 같습니다.
async fetchMovies(endPoint: string, queryParams: QueryParams) {
// data fetch 코드 생략...
if (response.status === 200) {
const data = await response.json();
this.totalPages = data.total_pages;
this.currentPage++;
return adaptMovieData(data)
}
export function adaptMovieData<T extends { genres?: movieGenre[] }>(
data: T
): T & { movieGenres?: movieGenre[] } {
if ("genres" in data && data.genres !== undefined) {
const movieGenres = data.genres;
const { genres, ...rest } = data;
return { ...rest, movieGenres } as T & { movieGenres?: movieGenre[] };
}
return data as T & { movieGenres?: movieGenre[] };
}
src/controllers/MovieModal.ts
Outdated
| 10: "명작이에요", | ||
| }; | ||
|
|
||
| function MovieModal(movieData: MovieDetailInfo, movieId: number) { |
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.
궁금증) moveId가 number 인데, 실제로 이 값이 DOM 요소에서 data-id로 들어오는 경우 string일 수 있는데 변환은 어디서 해주시는걸까요?
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.
string을 number 변환하는 코드는 작성하지 않았었습니다. 기존에 자바스크립트로 만든 부분 때문에 동작했던 것 같습니다.😂
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.
수정된 코드에서는 data-id를 number로 하지 않고 string으로 받아 사용하였습니다.
function MovieModal(movieData: MovieDetailInfo, movieId: string) {
src/controllers/MovieModal.ts
Outdated
| export const ratingDescriptions = { | ||
| 2: "최악이예요", | ||
| 4: "별로예요", | ||
| 6: "보통이에요", | ||
| 8: "재미있어요", | ||
| 10: "명작이에요", | ||
| }; |
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.
ts 를 사용하고 계시니까, Record<number, string> 에 대해서 공부해보고 여기에 적용하면 어떨지 알아보면 좋을거 같아요
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.
처음에 ratingDescriptions의 타입을 { [key: number]: string }으로 정의했었는데, 가독성측면에서 Record가 좋은 것 같아 Record<key, value>를 사용하여 수정했습니다!
export const ratingDescriptions: Record<number, string> = {
2: "최악이예요",
4: "별로예요",
6: "보통이에요",
8: "재미있어요",
10: "명작이에요",
};|
안녕하세요 케빈, 배포 링크 기입: 영화 리뷰 배포 페이지 수정된 부분
궁금한 점
혹시 위 방법 말고 케빈이 생각하시는 다른 해결 방법이 있을지 궁금합니다. |
-> 이런 문제가 있었군요.. 그래서 저도 고민해봤는데, 저같은 경우에는API마다 Adapter 함수 명시적으로 구분할거 같아요 이렇게 두어서 함수 수가 많아지는 단점은 있지만, 타입도 명확히 써서 추론 잘 되고, 테스트도 쉬운 장점이 있을거 같아요 |
JeongBin0227
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.
안녕하세요 앵버!
수정사항이 꽤 많았었다고 생각했는데 잘 반영해주시고, 앞으로 어떤걸 더 공부해야할지도 함께 명시해주셔서 좋았습니다 👍
이미 좋은 코드라고 생각해서 여기서 머지해도 될거 같아서 추가로 코멘트 몇개 더 적어드렸는데 읽어보시면 좋을거 같아요!
2단계 미션 고생많으셨습니다 💯 ~ 방학 잘보내세요~




안녕하세요 케빈,
이번 리뷰도 잘 부탁드립니다! 🙏
🎯 미션 소개
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
이벤트 핸들러를 이전 step1에서는 EVENT_HANDLER 라는 객체에 이벤트 동작 함수들을 정의했는데, step2에서 이벤트 동작들이 많아지면서 하나의 EVENT_HANDLER라는 객체에서 관리하기에 가독성도 떨어지고 관심사 분리도 제대로 안 되고 있다는 느낌을 받았습니다.
이를 해결하기 위해 각 관심사 별로 이벤트 핸들러 파일을 만들어 명확하게 분리하는 방향으로 리팩토링하려고 노력했습니다.
무한스크롤 구현 방식은 다양할 수 있지만 scroll 이벤트 방식과 IntersectionObserver이 대표적인 방법인 걸 알게되었습니다.
두 방식을 비교했을 때, IntersectionObserver가 성능적으로 더 유리하다는 점을 알 수 있었습니다. 특히, 구현에 있어서 IntersectionObserver가 좀 더 간결하게 구현이 가능한 것 같아서 IntersectionObserver를 사용해 무한 스크롤을 구현했습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
이번 과제에서 저장소 계층(예: localStorage 관련 로직)을 도메인과 분리하지 않았는데, 그 이유는 이번 과제가 상대적으로 기능이 단순하고 데이터 저장 방식도 localStorage로 제한되어 있어 복잡한 저장소 계층 분리 보다는 빠른 구현과 동작 확인에 집중하는 것이 우선이라고 생각했습니다. 다만, 과제가 복잡해진다면 각 계층이 자신의 책임에 집중하게 되어 코드가 더 깔끔해지고 유지보수하기 쉬워지기 때문에 저장소 계층을 분리할 필요성이 있을 것 같은데 케빈은 필요성이 있다고 생각하시는 지 또 필요성이 있다고 생각하신다면 어떤 방식으로 분리하시는 지 궁금합니다.
API 데이터를 수신하기 전에는 스켈레톤 UI를, 수신 후에는 영화 아이템을 보여주는 방식으로 구현했습니다. 기존의 스켈레톤 UI 방식대로 구현했지만, 데이터를 빠르게 받아오더라도 이미지 로드 여부에 따라 스켈레톤을 계속 보여주는 방식도 고려할 수 있다는 생각이 들었습니다. 결국, API 데이터를 받아 화면에 이미지를 렌더링하는 과정이 지연되어 사용자 경험이 저하될 수 있음을 깨달았습니다. 이러한 경우, API 데이터 자체에 의존하는 방식과 이미지 로드 상태에 의존하는 방식 중 어느 쪽이 사용자 경험 향상에 더 효율적인지에 케빈의 생각이 궁금합니다.
✅ 리뷰어 체크 포인트