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] 헤더 뒤로가기 기능 구현 #411

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

Conversation

Largopie
Copy link
Contributor

관련 이슈

작업 내용

사진으로 자세히 보고 싶거나, 구현 내용을 자세하게 파악하고 싶다면? -> 노션 링크 바로가기

각 페이지 별 헤더 사용처

뒤로가기 이동 경로 제목(title)
랜딩 페이지
약속 생성 페이지 → 첫 페이지라면 랜딩 페이지
→ 퍼널 패턴 이전 단계
"약속 만들기"
약속 생성 완료/공유 페이지 "약속 공유하기"
약속 입장 페이지 → 약속 생성 완료 페이지로 이동할까요? "약속 입장하기"
로그인 페이지 → 약속 입장 페이지 "로그인"
약속 조회 페이지 → 약속 입장 페이지 "약속 현황 조회"
약속 등록 페이지 → 약속 입장 페이지 "약속 등록하기"
약속 수정 페이지 → 약속 조회 페이지 "약속 변경하기"
약속 추천 페이지 → 약속 조회 페이지 "약속 추천받기"
약속 추천 페이지(주최자) → 약속 조회 페이지 "약속 확정하기"
약속 확정 페이지 "확정된 약속"
에러 페이지
Not Found 페이지

특이 사항

리뷰 요구사항 (선택)

@Largopie Largopie added 🐈 프론트엔드 프론트엔드 관련 이슈에요 :) 🚀 기능 기능을 개발해요 :) ♻️ 리팩터링 코드를 깎아요 :) labels Oct 16, 2024
@Largopie Largopie added this to the 6차(최종) 데모데이 milestone Oct 16, 2024
@Largopie Largopie self-assigned this Oct 16, 2024
Copy link

Test Results

34 tests   34 ✅  22s ⏱️
 4 suites   0 💤
 1 files     0 ❌

Results for commit 70381ce.

Copy link
Contributor

@Yoonkyoungme Yoonkyoungme left a comment

Choose a reason for hiding this comment

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

드디어 뒤로가기 기능이 생기는군요!!!!!!!!
고생하셨습니다 낙타
특히 백로그를 잘 작성해 주셔서 어떤 고민들을 했는지 쉽게 확인할 수 있었습니다👍🍑


[C]
router 처리 과정에서 제가 고민해본 방법을 공유할게요 :)
어제 디스코드에서 간단히 이야기 나눈 부분이지만, PR에도 기록으로 남기면 좋을 것 같아서 다시 언급해요!

Header가 필요한 페이지와 Header가 필요 없는 페이지를 구분하기 위해 GlobalLayout에서 조건부로 헤더를 렌더링하는 방법을 고민해봤습니다. 아래와 같이 useLocation을 활용한 방식은 페이지별로 유동적으로 헤더를 제어할 수 있어서 좋다고 생각합니다.

const location = useLocation();

// 헤더를 보여줄 경로들 ex
const HEADER_ROUTES = [
  '/meeting/create',
  '/meeting/:uuid/register',
  '/meeting/:uuid/viewer',
];

// 현재 경로가 헤더를 보여줄 경로들에 속하는지 확인
const showHeader = HEADER_ROUTES.some(route => location.pathname.startsWith(route));

이 방식을 사용하면, 특정 경로에 따라 헤더를 보여주거나 숨기는 처리가 가능해서 각 페이지별로 UI를 유연하게 제어할 수 있을 것 같아요. 특히 meeting 관련 페이지들에서 경로가 복잡해질 때, 이런 식으로 관리하면 유지보수도 쉬울 것으로 생각됩니다!

[Q]
위 제안에 대한 낙타의 의견이 궁금합니다💭

Copy link
Contributor

Choose a reason for hiding this comment

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

[P3]

layouts/GlobalLayout/index.tsx로 파일명 변경하는 건 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

같이 변경했습니다~!

Copy link
Contributor

Choose a reason for hiding this comment

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

[P3]

이 파일명도 index.tsx 바꾸는 건 어떨까요?
ContentLayout/index.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은 의견이에요 빙봉~! 🚀

@@ -0,0 +1,13 @@
import { useNavigate, useParams } from 'react-router-dom';

const useRouter = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

[P2]

useRouter라는 함수명이 뭔가 react-router-dom의 내장 함수와 혼동될 수도 있을 것 같아요. 좀 더 구체적인 이름을 사용해보면 좋을 것 같습니다 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

지금 당장 떠오르는 네이밍은 useCustomRouter인데, 이런 느낌으로 커스텀훅이라는 것을 드러내면 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분에 대한 상세한 답변은 위에 남겨두었습니다.

실제로 Next.js에서도 path를 이동시키기 위해 useRouter라는 이름을 사용하고 있네요!

Copy link
Contributor

Choose a reason for hiding this comment

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

좋습니다🎈🎈

import { useNavigate, useParams } from 'react-router-dom';

const useRouter = () => {
const router = useNavigate();
Copy link
Contributor

Choose a reason for hiding this comment

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

[P2]

현재 변수 router에는 useNavigate의 반환값을 담고 있네용! 변수명이 router 보다 navigate가 더 적절한 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

동의합니다. navigate가 조금 더 명확한 표현인 것 같네요 😄


const useRouter = () => {
const router = useNavigate();
const uuid = useParams<{ uuid: string }>().uuid;
Copy link
Contributor

Choose a reason for hiding this comment

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

[P3]

Suggested change
const uuid = useParams<{ uuid: string }>().uuid;
const { uuid } = useParams<{ uuid: string }>();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은데요~!

Copy link
Contributor

@Yoonkyoungme Yoonkyoungme Oct 18, 2024

Choose a reason for hiding this comment

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

[P2]

useRouter 훅을 만들어서 useNavigateuseParams를 결합한 이유는 meeting/:uuid/register, meeting/:uuid/viewer 등에서 URL 파라미터로 uuid를 반복적으로 추출하고 네비게이트하는 로직이 많기 때문으로 보입니다. 이 훅은 중복되는 코드를 줄이고 재사용성을 높이는 데 될 것 같네요👏

하지만 useNavigate는 이미 React Router에서 제공하는 네비게이션 함수로, 이걸 굳이 커스텀 훅으로 감싸서 사용할 필요가 있을지 의문이 듭니다. 단순히 경로를 변경하는 것은 navigate 내장 함수를 바로 사용하는 편이 더 명확할 것 같아요.

아래와 같이 특정 목적에 맞게 hook을 설계해보면 어떨까요?

import { useNavigate, useParams } from 'react-router-dom';

const useMeetingRouter = () => {
  const navigate = useNavigate();
  const { uuid } = useParams<{ uuid: string }>();

  const navigateToMeetingPage = (page: string) => {
    navigate(`/meeting/${uuid}/${page}`);
  };

  return { uuid, navigateToMeetingPage };
};

export default useMeetingRouter;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

하지만 useNavigate는 이미 React Router에서 제공하는 네비게이션 함수로, 이걸 굳이 커스텀 훅으로 감싸서 사용할 필요가 있을지 의문이 듭니다. 단순히 경로를 변경하는 것은 navigate 내장 함수를 바로 사용하는 편이 더 명확할 것 같아요.

저는 빙봉의 의견과 조금 다릅니다..!!

지금까지 관습적으로 navigate라는 변수명을 사용했기 때문에 navigate라는 단어에 익숙해진 것이 아닐까요?
useNavigate를 사용하는 훅에서 사용하는 변수명을 제한시켜 동일한 코드를 제공하기 위함도 해당 useRouter의 역할이라고 생각합니다.

만약 다른 개발자가 useNavigate()훅을 선언하는 변수명을 router라고 설정할 수도 있기 때문이에요.

마지막으로, uuid는 우리 서비스에서 사용처가 많기 때문에 별도로 추가한 부가기능입니다. uuid가 없는 페이지에서도 사용할 수 있는 유연함을 제공하고 싶었어요.

또한, uuid가 있는 경로에서 해당 훅을 사용하더라도 라우팅 경로는 '/'와 같이 원하는 곳으로 어디든 이동할 수 있다는 가정하에 선택해서 사용할 수 있도록 구현했습니다. 따라서 저는 useRouter라는 네이밍을 그대로 유지하고 싶은데 빙봉의 생각은 어떠하신가요?

만약 기존에 사용해왔던 navigate 변수를 동일하게 사용하고 싶다. 라고 하면 합의하에 routeTo가 아닌, navigate로 사용해볼 수 있을 것 같습니다!

Copy link
Contributor

@Yoonkyoungme Yoonkyoungme Oct 20, 2024

Choose a reason for hiding this comment

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

navigate라는 단어에 집중하기 보다는 경로를 변경하는 함수를 사용하는 방식이 현재 2가지라는 점에서 드렸던 코멘트입니다.
제가 이해한 바로는 낙타가 구현한 useRouter 훅에서 routeTo 함수는 useNavigate를 통해 경로를 변경하는 역할을 하고 있는 것 같아요.
현재 코드에서 보면, useNavigate를 사용하는 방식과 useRouterrouteTo 함수를 사용하는 방식이 서로 혼용될 수 있을 것 같습니다.

  • 예시 1: useNavigate를 직접 사용하는 코드
    const navigate = useNavigate();
    <button onClick={() => navigate("/")} />
  • 예시 2: useRouterrouteTo를 사용하는 코드
    const { routeTo } = useRouter();
    <button onClick={() => routeTo(`/meeting/${uuid}`)} />

결국 두 방식은 동일한 기능을 제공하지만, 중복된 방식으로 네비게이션을 처리하는 문제가 발생할 수 있을 것 같아요. (현재 모모 코드에서도 예시 1번처럼 사용하는 경우도 있고, 예시 2번처럼 사용하는 경우도 있는 것 같네요.)
특히 useNavigate는 React Router에서 이미 제공되는 함수인데, 이를 다시 커스텀 훅으로 감싸는 이유가 여전히 모호하게 느껴집니다.

저는 routeTo 함수가 useNavigate 함수 기능 외의 역할을 하고 있으면 남겨두는 것이 의미가 있지만, 그렇지 않다면 삭제하면 어떨까 하는 의견입니다.

Comment on lines 70 to 116
<>
<Header title="로그인">
{/* 현재 로그인 페이지는 빙봉이 구현한 로직 상 약속 입장 페이지에서만 사용됩니다. 따라서 뒤로가기 시, 입장 페이지로 이동하도록 구현했습니다. (@낙타) */}
<button css={s_backButton} onClick={() => routeTo(`/meeting/${uuid}`)}>
<BackSVG width="24" height="24" />
</button>
</Header>
<ContentLayout>
<div css={s_container}>
<div css={s_inputContainer}>
<Field>
<Field.Label id="닉네임" labelText="닉네임" />
<Field.Description description={FIELD_DESCRIPTIONS.nickname} />
<Input
placeholder="닉네임을 입력하세요."
value={attendeeName}
onChange={handleAttendeeNameChange}
/>
<Field.ErrorMessage errorMessage={attendeeNameErrorMessage} />
</Field>

<Field>
<Field.Label id="비밀번호" labelText="비밀번호" />
<Field.Description description={FIELD_DESCRIPTIONS.password} />
<Input
type="number"
id="비밀번호"
inputMode="numeric"
pattern="[0-9]*"
placeholder="비밀번호를 입력하세요."
value={attendeePassword}
onChange={handleAttendeePasswordChange}
/>
<Field.ErrorMessage errorMessage={attendeePasswordErrorMessage} />
</Field>
</div>
<Button
variant="primary"
size="full"
onClick={handleLoginButtonClick}
disabled={!isFormValid()}
>
로그인
</Button>
</div>
</ContentLayout>
</>
Copy link
Contributor

Choose a reason for hiding this comment

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

[NOTICE]

@Largopie @hwinkr
해리가 작업하고 있는 인풋 개선과 충돌날 것 같네요. 두 분 다 인지하고 계셔용~

Comment on lines 45 to 46
// 새로고침 시, 값을 받아오는 형태를 알 수 없으므로 null에 대한 처리: 기본값 설정 (@낙타)
if (!currentMeetingType) return HttpResponse.json(meetingEntranceDateTimeInfo, { status: 200 });
Copy link
Contributor

@Yoonkyoungme Yoonkyoungme Oct 18, 2024

Choose a reason for hiding this comment

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

[P3]

굿👍 중복되는 코드가 있어서 아래처럼 코드 수정 제안드립니다~

Suggested change
// 새로고침 시, 값을 받아오는 형태를 알 수 없으므로 null에 대한 처리: 기본값 설정 (@낙타)
if (!currentMeetingType) return HttpResponse.json(meetingEntranceDateTimeInfo, { status: 200 });
http.get(`${BASE_URL}/:uuid/home`, () => {
if (!currentMeetingType || currentMeetingType === 'DATETIME') {
return HttpResponse.json(meetingEntranceDateTimeInfo, { status: 200 });
} else if (currentMeetingType === 'DAYSONLY') {
return HttpResponse.json(meetingEntranceDaysOnlyInfo, { status: 200 });
}
}),

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

@hwinkr hwinkr left a comment

Choose a reason for hiding this comment

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

고생하셨습니다! 낙타~

저희 서비스가 언제 이렇게 페이지들이 많아졌는지 모르겠는데, 헤더에 페이지별로 고민한 흔적이 인상깊네요. 특히 노션에 정리해 놓은 것을 보니 리뷰하기 편했습니다.

사소한 변경 사항을 요청드리려고 합니다~ 나중에 다시 봐용 ㅎ

@@ -15,5 +15,5 @@ export const s_toastListContainer = css`
width: 100%;
max-width: 43rem;
padding: 1.6rem;
padding: 0 1.6rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

[C]

ㅎㅎ..🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

[P1]

해당 tsx 파일명을 index.tsx로 수정해주셔야 할 것 같습니다! 폴더, 파일명 컨벤션에 맞지 않는 것 같아요 현재는 ㅎㅎ

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 3 to 11
const useRouter = () => {
const router = useNavigate();
const uuid = useParams<{ uuid: string }>().uuid;

return {
uuid,
routeTo: (path: string) => router(path),
};
};
Copy link
Contributor

Choose a reason for hiding this comment

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

[P3]

  1. useRouter, useUuid 두 개로 커스텀 훅을 분리해보시는 것에 대해서 어떻게 생각하시나요? 관심사가 다른 커스텀 훅이라서, 분리를 하면 더 유연하게 사용할 수 있을 것 같아요. 예를 들어서, uuid만 알고 싶고 라우팅에 대한 책임은 없는 커스텀 훅이나 컴포넌트에서는 uuid를 알기 위해서 useRouter 훅을 호출해야할 것 같다는 생각이 들어서, 해당 방법을 추천드립니다!

  2. useRouter 훅 내부에 router(-1)을 호출하는, goBack 함수를 추가로 만들어 보는 것은 어떨까요? 추상화 + 직관적인 느낌을 줄 수 있을 것 같아 의견을 제시해 봅니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

의견 제안 감사합니다 :)

useRouter, useUuid 두 개로 커스텀 훅을 분리해보시는 것에 대해서 어떻게 생각하시나요?

조금 더 추상화해본다면 해리의 의견이 정확히 맞는 것 같아요. 필요하면 선택적으로 사용할 수 있게 하기 위함의 목적으로 uuid를 선언한 것이 맞습니다. 하지만, 역시 두 관심사가 다르니 충분히 분리를 고려해볼만하네요!

useRouter 훅 내부에 router(-1)을 호출하는, goBack 함수를 추가로 만들어 보는 것은 어떨까요?

이 부분도 매우 동의합니다. 추가해볼게요~!

Copy link
Contributor

Choose a reason for hiding this comment

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

[P2]

// 토스트 컴포넌트는 UI를 보여주는 책임만 가질 수 있도록 최대한 책임을 분리하고 스토리북을 활용한 UI 테스트를 쉽게할 수 있도록 한다.(@해리)
export default function Toast({ isOpen, type = 'default', message }: ToastProps) {
  const ToastIcon = iconMap[type];

  return (
    <div css={s_toastContainer(isOpen)}>
      {type !== 'default' && (
        <div css={[s_iconContainer, s_iconBackgroundColor(type)]}>
          {ToastIcon && <ToastIcon width={16} height={16} **stroke={theme.colors.white}** />} // 여기
        </div>
      )}
      <p css={s_toastText}>{message}</p>
    </div>
  );
}

stoke 속성을 사용하면 하나의 svg 파일을 추가로 만들지 않고 색을 재사용할 수 있어요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

변경된 아이콘의 경우에는 width, height가 동일하지만, 변경되지 않은 SVG의 경우에는 width, height가 다른 것을 볼 수 있습니다.

보통의 아이콘이라고 했을 때 높이와 너비는 대부분 동일하게 설정해주는 것으로 알고 있는데 변경 사항 전 SVG 형태에 width, height를 같게 해주었을 때 기존의 형태보다는 뭉개지거나 늘어진 형태로 아이콘이 보일 수 있다고 생각합니다.

<Check width="12" height="12" stroke={theme.colors.black} />

이와 같은 상황을 대비하여 현재의 svg 태그에 fill="current" 추가하여 사용처에서 fill 속성으로 색상을 지정할 수 있도록 하는건 어떨까요?

<Check width="12" height="12" fill={theme.colors.black} />
<svg width="current" height="current" viewBox="0 0 24 24" fill="current" xmlns="http://www.w3.org/2000/svg">
<path d="M9.55061 17.5754C9.41728 17.5754 9.29228 17.5544 9.17561 17.5124C9.05894 17.4711 8.95061 17.4004 8.85061 17.3004L4.55061 13.0004C4.36728 12.8171 4.27961 12.5794 4.28761 12.2874C4.29628 11.9961 4.39228 11.7587 4.57561 11.5754C4.75894 11.3921 4.99228 11.3004 5.27561 11.3004C5.55894 11.3004 5.79228 11.3921 5.97561 11.5754L9.55061 15.1504L18.0256 6.67539C18.2089 6.49206 18.4466 6.40039 18.7386 6.40039C19.0299 6.40039 19.2673 6.49206 19.4506 6.67539C19.6339 6.85872 19.7256 7.09606 19.7256 7.38739C19.7256 7.67939 19.6339 7.91706 19.4506 8.10039L10.2506 17.3004C10.1506 17.4004 10.0423 17.4711 9.92561 17.5124C9.80894 17.5544 9.68394 17.5754 9.55061 17.5754Z" fill="current"/>
</svg>

Comment on lines 73 to 75
<button css={s_backButton} onClick={() => routeTo(`/meeting/${uuid}`)}>
<BackSVG width="24" height="24" />
</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

[P2]

로그인 페이지가 입장 페이지에서만 사용된다면, 뒤로가기 버튼을 클릭했을 때,

routerTo(-1)

로 호출해 주면 되지 않을까요? useRouter 커스텀 훅 내부에서 뒤로가기 함수도 제공해 주면 될 것 같은데 어떻게 생각하시나요?

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.

저의 개인적인 의견으로 -1을 사용하는 것은 많이 위험이 도사리고 있다고 생각합니다.
가장 큰 이유가 직접적으로 url에 접근한 경우를 생각해볼 수 있을 것 같은데요..! 그 때 뒤로가기를 눌렀을 때 개발자가 예상한대로 동작하지 않기 때문에 명시적인 경로로 기입해두었습니다!

Comment on lines 41 to 43
<button css={s_backButton} onClick={() => navigate(-1)}>
<BackSVG width="24" height="24" />
</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

[P2]

<button css={s_backButton} onClick={() => navigate(-1)}>
    <BackSVG width="24" height="24" />
</button>

->

<BackButton />

뒤로 이동 시키는 버튼 컴포넌트를 추상화해 보는 것은 어떨까요??

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 45 to 46
// 새로고침 시, 값을 받아오는 형태를 알 수 없으므로 null에 대한 처리: 기본값 설정 (@낙타)
if (!currentMeetingType) return HttpResponse.json(meetingEntranceDateTimeInfo, { status: 200 });
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

@Largopie Largopie left a comment

Choose a reason for hiding this comment

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

@hwinkr @Yoonkyoungme

리뷰 반영사항 반영하고 다시 요청합니다~!
변경된 자세한 사항은 노션 하단에 해빙 리뷰 후 반영사항 에서 확인할 수 있습니다.

uuid 반환 로직 Context API & Layout 구성

// UuidProvider.tsx
export const UuidContext = createContext<UuidState>({} as UuidState);

export default function UuidProvider({ children }: PropsWithChildren) {
  const { uuid } = useParams();

  return <UuidContext.Provider value={{ uuid: uuid ?? '' }}>{children}</UuidContext.Provider>;
}
// UuidLayout.tsx
export default function UuidLayout() {
  return (
    <UuidProvider>
      <Outlet />
    </UuidProvider>
  );
}

// router.tsx

{
    path: ':uuid',
    element: <UuidLayout />,
    children: [
    // ...

BackButton 컴포넌트 추상화

위와 같이 버튼 컴포넌트를 추상화하고 반영했습니다. 여기서 더 이상 "약속 변경하기" 즉, 약속 수정 모드에서는 뒤로 가기 기능을 사용할 수 없습니다.(이유는 노션에 자세히 기록해두었습니다.)

interface BackButtonProps {
  path?: string;
}

export default function BackButton({ path }: BackButtonProps) {
  const { routeTo, goBack } = useRouter();

  const handleBackButtonClick = () => {
    if (path === undefined) goBack();
    else routeTo(path);
  };

  return (
    <button css={s_backButton} onClick={handleBackButtonClick}>
      <BackSVG width="24" height="24" />
    </button>
  );
}

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

변경된 아이콘의 경우에는 width, height가 동일하지만, 변경되지 않은 SVG의 경우에는 width, height가 다른 것을 볼 수 있습니다.

보통의 아이콘이라고 했을 때 높이와 너비는 대부분 동일하게 설정해주는 것으로 알고 있는데 변경 사항 전 SVG 형태에 width, height를 같게 해주었을 때 기존의 형태보다는 뭉개지거나 늘어진 형태로 아이콘이 보일 수 있다고 생각합니다.

<Check width="12" height="12" stroke={theme.colors.black} />

이와 같은 상황을 대비하여 현재의 svg 태그에 fill="current" 추가하여 사용처에서 fill 속성으로 색상을 지정할 수 있도록 하는건 어떨까요?

<Check width="12" height="12" fill={theme.colors.black} />
<svg width="current" height="current" viewBox="0 0 24 24" fill="current" xmlns="http://www.w3.org/2000/svg">
<path d="M9.55061 17.5754C9.41728 17.5754 9.29228 17.5544 9.17561 17.5124C9.05894 17.4711 8.95061 17.4004 8.85061 17.3004L4.55061 13.0004C4.36728 12.8171 4.27961 12.5794 4.28761 12.2874C4.29628 11.9961 4.39228 11.7587 4.57561 11.5754C4.75894 11.3921 4.99228 11.3004 5.27561 11.3004C5.55894 11.3004 5.79228 11.3921 5.97561 11.5754L9.55061 15.1504L18.0256 6.67539C18.2089 6.49206 18.4466 6.40039 18.7386 6.40039C19.0299 6.40039 19.2673 6.49206 19.4506 6.67539C19.6339 6.85872 19.7256 7.09606 19.7256 7.38739C19.7256 7.67939 19.6339 7.91706 19.4506 8.10039L10.2506 17.3004C10.1506 17.4004 10.0423 17.4711 9.92561 17.5124C9.80894 17.5544 9.68394 17.5754 9.55061 17.5754Z" fill="current"/>
</svg>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

하지만 useNavigate는 이미 React Router에서 제공하는 네비게이션 함수로, 이걸 굳이 커스텀 훅으로 감싸서 사용할 필요가 있을지 의문이 듭니다. 단순히 경로를 변경하는 것은 navigate 내장 함수를 바로 사용하는 편이 더 명확할 것 같아요.

저는 빙봉의 의견과 조금 다릅니다..!!

지금까지 관습적으로 navigate라는 변수명을 사용했기 때문에 navigate라는 단어에 익숙해진 것이 아닐까요?
useNavigate를 사용하는 훅에서 사용하는 변수명을 제한시켜 동일한 코드를 제공하기 위함도 해당 useRouter의 역할이라고 생각합니다.

만약 다른 개발자가 useNavigate()훅을 선언하는 변수명을 router라고 설정할 수도 있기 때문이에요.

마지막으로, uuid는 우리 서비스에서 사용처가 많기 때문에 별도로 추가한 부가기능입니다. uuid가 없는 페이지에서도 사용할 수 있는 유연함을 제공하고 싶었어요.

또한, uuid가 있는 경로에서 해당 훅을 사용하더라도 라우팅 경로는 '/'와 같이 원하는 곳으로 어디든 이동할 수 있다는 가정하에 선택해서 사용할 수 있도록 구현했습니다. 따라서 저는 useRouter라는 네이밍을 그대로 유지하고 싶은데 빙봉의 생각은 어떠하신가요?

만약 기존에 사용해왔던 navigate 변수를 동일하게 사용하고 싶다. 라고 하면 합의하에 routeTo가 아닌, navigate로 사용해볼 수 있을 것 같습니다!

@@ -0,0 +1,13 @@
import { useNavigate, useParams } from 'react-router-dom';

const useRouter = () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분에 대한 상세한 답변은 위에 남겨두었습니다.

실제로 Next.js에서도 path를 이동시키기 위해 useRouter라는 이름을 사용하고 있네요!

import { useNavigate, useParams } from 'react-router-dom';

const useRouter = () => {
const router = useNavigate();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

동의합니다. navigate가 조금 더 명확한 표현인 것 같네요 😄


const useRouter = () => {
const router = useNavigate();
const uuid = useParams<{ uuid: string }>().uuid;
Copy link
Contributor Author

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.

좋은 의견이에요 빙봉~! 🚀

Copy link
Contributor Author

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 73 to 75
<button css={s_backButton} onClick={() => routeTo(`/meeting/${uuid}`)}>
<BackSVG width="24" height="24" />
</button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

저의 개인적인 의견으로 -1을 사용하는 것은 많이 위험이 도사리고 있다고 생각합니다.
가장 큰 이유가 직접적으로 url에 접근한 경우를 생각해볼 수 있을 것 같은데요..! 그 때 뒤로가기를 눌렀을 때 개발자가 예상한대로 동작하지 않기 때문에 명시적인 경로로 기입해두었습니다!

Comment on lines 41 to 43
<button css={s_backButton} onClick={() => navigate(-1)}>
<BackSVG width="24" height="24" />
</button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

그것도 굉장히 좋은 생각이네요 해리~ 추상화 해보겠습니다~!

해리는 추신(추상화의 신)이네요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
♻️ 리팩터링 코드를 깎아요 :) 🐈 프론트엔드 프론트엔드 관련 이슈에요 :) 🚀 기능 기능을 개발해요 :)
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

[FE] 뒤로가기 기능 구현
3 participants