Skip to content
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

강의 정렬순 버튼 관련 이슈 해결 #106

Merged
merged 12 commits into from Nov 12, 2021
Merged

Conversation

hyoni0817
Copy link
Collaborator

수정한 사항

  • addView 부분 코드에서 url 쿼리 스트링에 view가 추가될 때와 그렇지 않을 때의 조건을 다시 생각해서 코드를 수정했습니다.
  • 이해가 쉽게 되지 않는 부분에 대해 주석을 추가했습니다.
  • handleOrderChange의 e에 React.ChangeEventHandler 타입 정의 후 e.target.value의 target 부분에 Property 'target' does not exist on type 'ChangeEventHandler'. 에러나는 부분을 토니님이 알려주신 방법을 통해 해결했습니다.

추가 사항

  • view나 order 버튼 기능 이용 후, 상단 메뉴에서 '강의'를 다시 눌렀을 때 order와 view의 값이 여전히 유지되고 있는 문제 해결을 위해서 검색 필터 초기화 버튼을 추가했습니다.

앞으로 진행할 작업

  • 기술 검색 부분 구현하기

@hyoni0817 hyoni0817 added this to In progress in Inflearn-front via automation Nov 8, 2021
@TaehwanGo
Copy link
Member

TaehwanGo commented Nov 9, 2021

image

초기화 버튼 눌렀을 때 로딩스피너 나오는 것은 좋지만
main 컴포넌트가 높이가 없어져서
footer가 올라왔다가 다시 내려가는게 사용자 경험이 안좋은 것 같아요
로딩중에 미리 사이즈를 잡아 놓는 것도 좋을 것 같아요 🙂

@TaehwanGo
Copy link
Member

image
image
image

버튼들의 높이가 다 달라서(41px, 38px, 40px) 보기에 살짝 아쉬운 것 같아요
버튼들의 높이가 통일 되면 좋을 것 같아요 🙂

Copy link
Member

@TaehwanGo TaehwanGo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

router에 shallow 옵션이 있는 것을 새롭게 알게 되어 좋았어요 :)
초기화를 위해 고민한 흔적이 많이 보여서 좋았어요 👍


**시도한 해결 방법 (2)**

HeaderLayout.tsx에서 `<Link href={href}>` 대신에 `router.push(href)`를 사용해보고 next.js API 문서를 참고해서 혹시나 하는 마음에 Shallow 옵션도 router에 추가해봤지만 해결이 되지 않았다.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shallow option을 router에 추가하는 것과 추가하지 않는 것은 어떤 차이가 있나요?
찾아보니 getServerSideProps에서 data-fetching하는 것을 하지 않고도 url을 변경할 수 있다고 하는데
이것을 만약에 제가 만들고 있던 CourseLayout.tsx에 적용하고
useEffect로 가져오던 데이터를 getServerSideProps로 가져오게 한 다음
url 이동시 사용하는 router.push()에 { shallow: true}를 전달하면
리덕스로 매번 페이지 변경 시 마다 요청하던 것이 없어질 수도 있을 것 같네요
적용해보고 코멘트 남길게요 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

버튼 크기 관련해서 좋은 지적 감사합니다 40px로 높이를 다 맞췄습니다 :) 눈으로만 확인한 후 똑같다고 생각하여 신경 쓰지 못한 채 지나친 부분인데 자세히 확인해주셔서 감사합니다!😊 앞으로는 이런 부분들도 잘 확인하도록 하겠습니다 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 로딩 스피너와 관련하여 높이를 잡는 부분에 대해 좋은 의견을 주셔서 감사합니다! :) 구현에만 신경을 쓰느라 이 부분의 사용자 경험에 대한 생각을 놓치고 있었네요😅

로딩이 되는 부분의 높이를 잡다보니 강의 카드 갯수에 따라 높이가 달라지는 경우에는 어떻게 고정을 시키는 방법이 좋을지 고민이 됩니다.
강의 갯수가 제일 많을 때의 높이로 고정하면 강의 갯수가 없을 때는 불필요한 스크롤을 많이 하게 되고, 좀 더 작은 높이로 고정하면 토니님이 의견 주신 내용과 같이 계속 footer가 움직이는 것 처럼 보이는 문제가 생깁니다.
그래서 인프런처럼 강의 카드 위에 로딩 스피너를 보이게끔 해야하는 방법을 사용해야 할 것 같다는 생각이 듭니다.

Copy link
Member

@TaehwanGo TaehwanGo Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 맞아요 인프런처럼 absolute로 띄워서 놓는 것도 괜찮을 것 같아요
아니면 다른 웹사이트(유튜브) 보면
image
이렇게 미리 공간을 잡아놓는 방식도 있더라구요

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아, 맞아요! 스켈레톤 방식도 생각을 해봤는데 데이터를 몇 개 들고올지 모르는 상태에서는 이 방식을 사용하는 게 맞는지 고민이 되더라구요. 예를 들면 스켈레톤으로 24개를 미리 보기로 보여주는데 서버에서 받아오는 강의 데이터는 3개 밖에 없다거나 할 때요 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런 방식이 스켈레톤 방식인가요? ㅎㅎ 이번에 제대로된 용어를 배웠네요
컨텐츠가 많아지면 위 스켈레톤 방식이 좋은 것 같고
아니면 아까 말씀하신대로 인프런처럼 로딩스피너를 카드 위에 띄우는 것도 좋을 것 같네요 :)
만약에 나현님이 둘중에 하나로 구현한다면 저는 다른쪽에서 다른방식으로 구현해볼게요
예를들어 나현님이 로딩스피너 방식으로 하신다면
제가 메인페이지에 스켈레톤 방식을 적용해보는 식으로요 🙂

Copy link
Collaborator Author

@hyoni0817 hyoni0817 Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네! :) 제가 둘 중에 어떤 게 나은지 비교해보고 다음 pr 때 추가해서 올리겠습니다 :) 좋은 의견 감사합니다!👍

그리고 shallow 옵션을 사용하면 새로고침없이 url이 변경되는데 url에 있는 query를 useEffect의 deps 배열에 추가해서 리렌더링도 되게끔 할 수 있는 것 같아요! 근데 당장 문제 해결하고 싶은 마음에 api 문서를 대충 번역해서 적용을 하여 문제가 잘 해결이 안 된 것 같아서 지금 찾아 보면서 반성하게 되네요😅 좋은 의견 주셔서 감사합니다! :)
이 부분은 내일 제가 한 번 더 코드 수정을 한 후에 merge를 하도록 하겠습니다 :)
아! 위에 남겨주신 코멘트에서 getServerSideProps에 shallow 옵션을 적용해보신다고 말씀해주셨는데 merge를 하게 되더라도 브랜치는 남겨두겠습니다 :)

Copy link
Member

@TaehwanGo TaehwanGo Nov 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

괜찮습니다.
머지하시고 브랜치 삭제하셔도 될거같아요 :)
shallow는 저도 좀 더 알아봐야될 것 같아요
getServerSideProps 를 쓰려니까 hooks를 사용 못 하는 것 같더라구요
그래서 dispatch를 useDispatch를 사용하지 않고 사용하거나 아니면 getServersideProps의 올바른 사용예를 검색해서 올려볼게요
이것과 관련된 이슈입니다. #107

dispatch({
type: SEARCH_LECTURES_REQUEST,
data: queryList.current,
});
}, []);

useEffect(() => {
// unmount시 useRef로 관리하는 값들을 초기화
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분을 추가했는데 시도한 방법 (1)에서
헤더의 강의 메뉴를 눌러도 unmount가 실행되지 않아서 효과가 없었다는 건가요? 🙂

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네! 강의 메뉴를 눌렀을 때 리렌더링이 발생하는데, 이때 useRef는 리렌더링이 되어도 current에 저장된 값을 계속 유지한다는 것을 이번에 알게 되었습니다. 대신에 아예 페이지를 벗어난 경우여야 unmount되어 useRef가 초기화 되더라구요 :)

@@ -254,6 +291,23 @@ const Courses = () => {
queryOrder.current = result;
}, []);

const handleResetClick = useCallback(() => {
// queryView와 queryList 값이 없는 경우에는 초기화 버튼을 실행시키지 않음으로써 불필요한 서버 요청을 줄임.
if (!queryView.current && !Object.keys(queryList.current).length) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불필요한 서버 요청을 줄이기 위한 로직 좋네요 ! 👍

@hyoni0817 hyoni0817 merged commit 2b5cbe4 into main Nov 12, 2021
Inflearn-front automation moved this from In progress to Done Nov 12, 2021
@hyoni0817 hyoni0817 deleted the courses/order-btn-update branch November 12, 2021 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants