-
Notifications
You must be signed in to change notification settings - Fork 0
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
KDT5_이시우_2차과제_RE #58
base: main
Are you sure you want to change the base?
KDT5_이시우_2차과제_RE #58
Conversation
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를 사용하는 방법을 배우셨다면, 이제는 연관성이 있는 로직을 컴포넌트에서 분리, 하나의 파일로 모으는 작업(api.jsx
), 컴포넌트를 잘게 쪼개어 책임을 분리하는 작업을 진행해보시면 좋을 것 같습니다. 이러한 작업에 있어서 생각해볼만한 부분 코멘트 드렸습니다.
확인해보시고 리플 달아주세요!
<link | ||
rel="preconnect" | ||
href="https://fonts.googleapis.com" /> | ||
<link |
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.
preconnect를 사용하여 font 파일을 들고오고 있네요:) 해당 속성이 어떠한 의미로 사용되고 있는지 또 어떤 효과가 있어서 추가한 것인지 설명해주실 수 있을까요?
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.
사용할 당시에는 그대로 가져와 사용해 인지하지 못했는데 멘토님의 리뷰를 보고 찾아보게되어 리플남깁니다!
rel = preconnect
를 사용하게 되면 페이지가 로드 될 때 외부리소스의 도메인에 미리 연결을 하게 되어, 리소스 로드 시간을 줄여주는 속성 값입니다.
rel = preconnect
속성값을 사용하지 않았을때 웹폰트가 로딩 되기 전까지 텍스트가 나오지 않거나 일부만 나오는 현상이 나타나 폰트가 깜빡이는 듯한 느낌을 줄 수 있는 문제가 있어 이를 방지하고 웹폰트를 유연하게 사용하기위해 쓰입니다! 🥸
import axios from 'axios' | ||
|
||
export async function fetchMovies(inputText, selectedType, page) { | ||
const { data } = await axios({ |
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.
axios 도입 및 HTTP 요청 로직을 상태관리 로직과 분리한 것은 매우 훌륭합니다. 더 나아가 axios의 get()
메서드 도입을 추가적으로 고려해보면 좋을 것 같아요. 현재 axios()
를 통해서 사용하고 있으나, axios에서는 HTTP 요청의 방식인 method
(GET
)에 대응되는 get()
를 제공하고 있기 때문입니다.
이 메서드를 사용해야하는 이유는 해당 메서드를 사용하는 것을 만으로도 해당 요청이 GET
방식이라는 것을 알 수 있으며, 아래의 그림과 같이 '?' 뒤에 들어가는 여러가지 값들을 객체의 형태로 전달할 수 있기 때문입니다. 지금과 같이 url의 문자열에 추가할 수 있지만 나중에 `? 뒤에 추가해야하는 값이 20-30개면 문자열이 너무나도 길어질 수 있겠죠? axios docs 링크를 남겨두었으니 한번 읽어보시는 것을 추천드립니다.
axios.get('https://omdbapi.com', {
params: {
apikey: '7035c60c',
s: inputText,
type: selectedType,
page: page
}
})
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.
감사합니다!! 문서 참고하여 리팩토링시 수정하겠습니다!
<NavLink | ||
to="/" | ||
className="logo"> | ||
<span className="logo--color">Mov</span>iecu <span>!</span> |
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.
BEM 방법론을 잘 적용하고 계시네요👍🏻 여기 logo--color
의 클래스명이 해당 태그에 대해서 색을 부여하기 위함으로 보이는데, 혹시 실제 색이 반영되는 내부 span
태그가 아니라 현재 태그에 logo--color
의 클래스명을 사용한 이유가 있을까요?
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.
앗...! 이 부분을 제가 footer 로고 부분을 진행하면서 알게되어 수정했었는데제가 header 로고 부분에는 변경을 못해서.. 다소.... 번거롭게 색상이 변하는 부분이 아닌 그 앞뒤로 색상을 지정해 주었던것 같습니다..!
리팩토링 진행시 변경하도록 하겠습니다! 🥹🫠
@@ -0,0 +1,17 @@ | |||
import TheHeader from '~/components/TheHeader' |
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.
프로젝트를 vite react로 진행 하였기 때문에 vite에서 지원하는 (vite.config 폴더 안)alias
를 사용해서
path alias: [{ find: '~', replacement: ${__dirname}/src }]}
로 절대경로를 설정해 주었습니다!
e.target.src = xboxImg; | ||
}; | ||
|
||
async function fetchDetailsMovie() { |
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.
해당 HTTP 요청 로직 또한 API 파일로 옮기는 것이 좋을 것 같아요. 따로 컴포넌트 내에 두신 이유가 있을까요?
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.
앗 넵 따로 컴포넌트 내에 둔 이유는 없고, 마지막 컴포넌트 정리를 진행했어야 했는데 하지 못했던 것 같습니다!
리팩토링시 진행하도록 하겠습니다! 🙌🏼
} | ||
|
||
useEffect(() => { | ||
(async () => { |
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.
(async () => {
setMovie(await fetchDetailsMovie());
})();
위의 로직을 함수로 새로 만들지 않고 useEffect
내에 선언한 이유가 궁금합니다.
++ ( () => {} )()의 문법으로 익명함수를 만들어 즉시 실행하는 방식을 사용한 이유는 아마 useEffect() 내에서 직접적으로 async
가 불가능하기 때문이라고 생각하는데요. 해당 비동기 로직을 하나의 메서드로 구현하여 useEffect()
내에서 호출하면 되지 않을까요?🤔
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.
저는 개인적으로 useEffect()
상태값의 변화에 따라 로직을 호출하는 영역으로, 그 내부에는 최대한 로직의 선언을 줄여야 한다고 생각하는데요. 한 번 해당 useEffect()
의 로직들을 분리하여 메서드로 만들고 내부를 간소화하보는 것을 추천드립니다:)
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에 대한 공부가 부족했던것 같습니다🥹
멘토님이 말씀해주신 방법에대해서 공부해서 적용해 보도록 하겠습니다! 감사합니다!
</section> | ||
<section className="data-form"> | ||
<ul className="data-form__data"> | ||
{movies?.length < 1 && ( |
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.
넵..! 그부분까지 미처 생각을 해보지 못했던것 같습니다! 리팩토링 시 반영하여 보겠습니다! 🤗
one-time donation or become a patron. | ||
</p> | ||
</section> | ||
<section className="search-form"> |
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.
search-form
클래스명의 section
는 컴포넌트로 분리하여 관리하게 되면, 메인페이지의 역할인 Search
컴포넌트에서 검색 기능을 분리할 수 있을 것으로 보이는데, 어떻게 생각하시나요?🤔
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를 활용해 '완성 예시' 처럼 자유롭게 영화 검색 기능을 구현해보세요!
과제 수행 및 리뷰 기간은 별도 공지를 참고하세요!
🦹🏻 과제 링크
-----------🩵Moviecu🩵-----------
react로 진행하였고 필수 조건 구현 완료 하였고 추가 요소를 진행하였습니다만...
아직 수정중입니다...(노력ing)
🙉 과제 진행 목표
❗ 필수
❔ 선택
추후 보완해야하는 부분