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] swipe 기능을 ref를 받아 쓰도록 개선한다 #959

Merged
merged 8 commits into from
Nov 15, 2024

Conversation

skiende74
Copy link
Contributor

@skiende74 skiende74 commented Nov 14, 2024

❗ Issue

✨ 구현한 기능

  • 변경내용 : 기존 = useMouseDrag가 swipe 핸들러를 window에 걸었음 -> ref를 받아서 ref에 걸도록 변경
- 해결하고자 한 문제 : swipe 이벤트 때문에, 주소모달에서의 스와이프에도 탭이동이 되어버림.
- 해결 :  훅의 이벤트 핸들러 범위를 전역(window)대신 좁은 범위로 잡기
  • 변경내용 : 층수 입력 등 숫자형 방기본정보 입력시 모바일 숫자패드가 뜨도록 반영
- 해결하고자 한 문제 : 모바일에서 숫자패드가 뜨게 하고싶다. 그러나 type을 사용하면 stepper가 생기는 등 사이드이펙트가 존재
- 방법 : inputMode="decimal"사용
  • 변경내용 : (UI) 편집체크리스트의 체크리스트에도 탭이동버튼을 추가
- 원인 : 추가/편집이 서로다른컴포넌트를 쓰고있었음. 당시에 '추가'에만 넣어줬었음.
- 방법 : 그냥 추가해줌
  • post-signup을 fetch->fetcher로 대체하였습니다.

📢 논의하고 싶은 내용

🎸 기타

@skiende74 skiende74 changed the base branch from dev to dev-fe November 14, 2024 03:44
Copy link

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.

자잘한 코멘트 있어서 달아놨어용~!

Comment on lines 40 to +41
credentials: 'include',
headers: { 'Content-Type': 'application/json' },
// 쿠키전달이기때문에 body가 비어있는 post
Copy link
Contributor

Choose a reason for hiding this comment

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

fetcher 내부에 이미 credentials: 'include',이 포함되어 있어서 불필요한 코드인거 같네요~!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영했습니다.

body: JSON.stringify({ name, email, password }),
return await fetcher.post({
url: `${BASE_URL}${ENDPOINT.REGISTER}`,
body: { name, email, password },
credentials: 'include',
Copy link
Contributor

Choose a reason for hiding this comment

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

이부분도 동일합니다~!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영했습니다.

useMouseDrag(ref, (S, E) => {
const DRAG_THRESHOLD = 100;
const TAB_COUNT = DefaultChecklistTabsNames.length;
const remainOp = (a: number, b: number) => (((a % b) + b + 1) % b) - 1; // 나머지연산자. -1부터 시작하므로 +1 -1
Copy link
Contributor

Choose a reason for hiding this comment

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

유틸로 분리할 수 있을 수 있지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

밑에 자세히 작성하였습니다.

const { setCurrentTabId } = useTabContext();
const ref = useRef<HTMLElement>(null);
useMouseDrag(ref, (S, E) => {
const DRAG_THRESHOLD = 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분도 상수 분리되면 좋을거 같네용~~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

반영했습니다.

Comment on lines 19 to 28
useMouseDrag(ref, (S, E) => {
const DRAG_THRESHOLD = 100;
const TAB_COUNT = DefaultChecklistTabsNames.length;
const remainOp = (a: number, b: number) => (((a % b) + b + 1) % b) - 1; // 나머지연산자. -1부터 시작하므로 +1 -1
setCurrentTabId(tabId => {
const isLeftDrag = E.x - S.x > DRAG_THRESHOLD;
const isRightDrag = S.x - E.x > DRAG_THRESHOLD;
if (isLeftDrag) return remainOp(tabId - 1, TAB_COUNT);
if (isRightDrag) return remainOp(tabId + 1, TAB_COUNT);
return tabId;
Copy link
Contributor

Choose a reason for hiding this comment

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

위에 코드리뷰와 동일하게 동일한 코드가 반복되는거 같아서 유틸 분리, 상수 분리가 되면 좋을거 같아요~!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

확인했습니다.

@@ -11,6 +11,7 @@ const MaintenanceFee = () => {
<FormField.Label label="관리비" htmlFor="maintenanceFee" />
<FormStyled.FieldBox>
<Input
inputMode="decimal"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍👍👍

Copy link

Copy link

Copy link

@skiende74
Copy link
Contributor Author

skiende74 commented Nov 14, 2024

헤일리 리뷰 반영했습니다.
반영내용: TAB_THRESHOLD 상수파일로 이동, fetcher/credential.

추가로
useMouseDrag를 두 곳에서 쓰느라 5~6줄에걸친 코드가 반복된다는 점이 좀 걸려서 변경해보았습니다.

  • 변경내용: useMouseDrag를 이용해 100px 움직일시 탭이동하는 로직을 useTabContext내 에서 제공하게 변경
해결하고자 한 문제 : useMouseDrag를 두 곳에서 쓰느라 5~6줄에걸친 코드가 반복됨.
해당 반복코드를 useMouseDragForTab 같은 훅으로 뽑아서 사용하면, 나중에 또 탭이동을 이용할 일이 있을 때 useMouseDrag를 써야할 지, useMouseDragForTab을 써야할지 일일이 들여다 봐야함.
해결 : useTabContext 에서 제공함으로써 탭을 쓸 때, 그냥 사용할 수 있게 함

유틸 분리에 대해

  • remainOp을 유틸로 분리하는 리뷰에 대해
    단순히 util폴더로 이동 시 colocation을 해치고, (tab에서만 사용되는 로직인데 사용처와 멀어짐)
    그렇다고 내부에 그냥 놔두면 가독성에 아쉬운 면이 있다고 생각했습니다.
    이제는 한 곳에서만 쓰이는 함수인 만큼,
    상수파일로 이동하지는 않고, 훅 내부 대신 외부에 위치하게 두었습니다 (같은 파일).

@@ -21,6 +22,7 @@ const DepositAndRent = () => {
/>
<FormStyled.FlexLabel label=" / " />
<FormField.Input
inputMode="decimal"
Copy link
Contributor

Choose a reason for hiding this comment

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

이런 속성이 있는지는 처음 알았네요 제이드 덕분에 배우고 갑니다!!

export const TabProvider = ({ children, defaultTab = 0 }: Props) => {
const [currentTabId, setCurrentTabId] = useState<number>(defaultTab);

return <TabContext.Provider value={{ currentTabId, setCurrentTabId }}>{children}</TabContext.Provider>;
// 드래그로 탭이동을 위한 훅을 리턴에 제공
const useDragForTab = (dragThreshold: number = DRAG_THRESHOLD_PIXEL) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

TabContext에 넣으니까 응집성 있고 좋네요!!👍

@skiende74 skiende74 self-assigned this Nov 15, 2024
@skiende74 skiende74 merged commit 465eabd into dev-fe Nov 15, 2024
3 checks passed
@skiende74 skiende74 deleted the fix/956-swipe branch November 15, 2024 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants