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

[FE] 좋아요 필터링 기능을 구현한다. #817

Merged
merged 8 commits into from
Oct 18, 2024

Conversation

skiende74
Copy link
Contributor

@skiende74 skiende74 commented Oct 17, 2024

❗ Issue

✨ 구현한 기능

  • 좋아요 필터링을 위해 기존의 useGetChecklistListQuery를 감싸는 useGetChecklistList 훅 생성.

📢 논의하고 싶은 내용

체크리스트리스트 필요할땐 useGetChecklistListQuery 쿼리말고 이 훅을 써주세요.
좋아요 필터링 여부를 기억하고, 좋아요 필터링/해제를 할 수 있도록 하는 훅

🎸 기타

@skiende74 skiende74 self-assigned this Oct 17, 2024
@skiende74 skiende74 added this to the 마일스톤 8주차 milestone Oct 17, 2024
Copy link

const response = await fetcher.get({ url: BASE_URL + ENDPOINT.CHECKLISTS });
export const getChecklists = async (isLikeFiltered: boolean = false) => {
const response = await fetcher.get({
url: BASE_URL + (isLikeFiltered ? ENDPOINT.CHECKLISTS_LIKE : ENDPOINT.CHECKLISTS),
Copy link
Contributor

Choose a reason for hiding this comment

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

개인적으로 이렇게 다른 엔드포인드의 api 함수를 묶는 건 좋지 않은 선택인 것 같다는 생각이 드네요. 나중에 필터링 로직이 추가된다면, 해당 함수에 너무 많은 로직이 추가될 것 같은데(물론 제이드가 이랬다면 분리하셨을 것 같지만..) 미리 둘을 분리하는 건 어떠신가요 ?

Copy link
Contributor

@ooherin ooherin Oct 18, 2024

Choose a reason for hiding this comment

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

지금 생각해보니 취향 차이인 것 같긴한데, getChecklists 로부터 좋아요 fetch 를 유추할 수 있나? 가 고민인 것 같네요. 같은 get 메서드이니 상관없긴 하지만, 개인적으로는 �메서드 명에 필터링까지 드러나야 한다고 생각하긴 합니다..! 제이드의 의견이 궁금하네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

사실 지금 백엔드가 다른엔드포인트로 구별되어서그렇지
like 같은 필터링은,
통상 동일한 엔드포인트에서 query param만 ?fiter=like 이런식으로 넣어서 구현하는 경우가 대부분이라 생각합니다.

따라서 사용처 입장에서는 그게 별도의 엔드포인트라는 사실을 감추는 게 유리하다고 생각합니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

음 그렇게요 지금 형태도 괜찮은 것 같아요!

toggle: () => void;
};
}
export const useLikeFilterStore = create<IsLikeFilterEnabledState>((set, get) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

다른 LIKE 쿼리로 저장하는 것이 아닌 이렇게 store 를 선택한 이유가 있으신가요? 다른 api 와 달리 store 를 쓴 점에 대해 이유가 궁금합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like 상태에따라 두 쿼리를 바꿔서 사용해야하는데,
이 like 상태를 바꾸는 위치가 다른 컴포넌트라서

나이브하게 구현하면
like 여부를 기억하는 zustand store로 like 상태를 기억시키고.
사용처에서는 이 store상태에따라 다른 api 호출하는 로직 작성인데,

이러면 컴포넌트에 구현이 노출되는 셈이라 선언적으로 하기위해 훅으로 하였습니다.

Copy link
Contributor

@ooherin ooherin Oct 18, 2024

Choose a reason for hiding this comment

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

이해했습니다!! 호출부와 api 사용처가 달라서 생긴 이슈군요


const useGetChecklistList = () => {
const {
isEnabled,
Copy link
Contributor

Choose a reason for hiding this comment

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

isEnabled 으로 해당 like 가 필터링 됐는지 ,안됐는지 알기가 어려운데, 이름을 isLiked 나 isLikeFiltered 등으로 바꾸면 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isLikeFiltered 좋은거같습니다.

Copy link

Copy link
Contributor

@healim01 healim01 left a comment

Choose a reason for hiding this comment

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

확인했습니다!!
이후에는 구현 화면도 함께 본문에 추가해주시면 좋을거 같아용~!

@skiende74
Copy link
Contributor Author

skiende74 commented Oct 18, 2024

확인했습니다!! 이후에는 구현 화면도 함께 본문에 추가해주시면 좋을거 같아용~!

놓쳤었네요. 알겠습니다 !
image

모바일에서 제목이 2줄되는현상을 막기위해
"방 둘러볼 때 꼭 필요한 체크리스트" -> "내가 둘러본 방" 으로 고쳤는데
어떻게생각하시는지도 궁금합니다.

그리고 헤일리가 제목체크리스트 질문 버튼 아래로 내려주는식의 얘기를 해줬던 것 같기도한데 내리는게 나을까요?

@healim01
Copy link
Contributor

"방 둘러볼 때 꼭 필요한 체크리스트" -> "내가 둘러본 방" 으로 고쳤는데

흠.. 이 부분은 기존 워딩이 이 좋은거 같습니다.
저렇게 지정한 이유가 아티클과 더불어 메인페이지에서도 함께 맞춰서 정해진 워딩이라서요.

좋아요를 내리는 방식도 고려해보면 좋을거 같아요!

피그마에 대략 레이아웃 추가해놓겠습니다. 골라보면 좋을거 같아요!

@skiende74
Copy link
Contributor Author

skiende74 commented Oct 18, 2024

피그마에 대략 레이아웃 추가해놓겠습니다. 골라보면 좋을거 같아요!

좋은 방법인 것 같습니다.
다만 빠른 머지가 필요한 시점이라
(밀린 추후예정PR : 리안의 로그인작업, e2e 작업, 스토어 병합작업)

우선은 머지를 하되

"워딩" + "레이아웃" 안건을 이 PR에서 계속 논의 및 결정해서 그 논의 결과를
추후 PR에서 반영해도 괜찮을까요?

@ooherin
Copy link
Contributor

ooherin commented Oct 18, 2024

워딩에 관해서 개인적인 의견을 말씀 드리자면, 기존에 '방을 꼭 둘러볼때~' 라는 문구는 길기도 하고 홍보문구의 색이 강해서 제이드가 바꾼 버전이 더 나은 것 같습니다. 사실 홍보용도 나쁘지 않긴 한데, 저런 서브 타이틀에서는 직관성, 이해성이 먼저라고 생각해서요. '내가 둘러본 방' => '체크리스트 리스트' 가 더 잘 이어지는 것 같아요!

Copy link

@skiende74
Copy link
Contributor Author

skiende74 commented Oct 18, 2024

#833
레이아웃 관련 이슈를 파두었으니 추후 대화는 새 이슈에서 해주세요 !

@skiende74 skiende74 merged commit 59c0c97 into dev-fe Oct 18, 2024
3 checks passed
@skiende74 skiende74 deleted the feat/811-like-filtering-impl branch October 18, 2024 04:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants