-
Notifications
You must be signed in to change notification settings - Fork 8
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
[FE] 헤더 뒤로가기 기능 구현 #411
Conversation
Test Results34 tests 34 ✅ 24s ⏱️ Results for commit e160ed2. ♻️ This comment has been updated with latest results. |
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.
드디어 뒤로가기 기능이 생기는군요!!!!!!!!
고생하셨습니다 낙타
특히 백로그를 잘 작성해 주셔서 어떤 고민들을 했는지 쉽게 확인할 수 있었습니다👍🍑
[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]
위 제안에 대한 낙타의 의견이 궁금합니다💭
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.
[P3]
layouts/GlobalLayout/index.tsx
로 파일명 변경하는 건 어떨까요?
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.
[P3]
이 파일명도 index.tsx 바꾸는 건 어떨까요?
ContentLayout/index.tsx
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.
좋은 의견이에요 빙봉~! 🚀
@@ -0,0 +1,13 @@ | |||
import { useNavigate, useParams } from 'react-router-dom'; | |||
|
|||
const useRouter = () => { |
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.
[P2]
useRouter
라는 함수명이 뭔가 react-router-dom
의 내장 함수와 혼동될 수도 있을 것 같아요. 좀 더 구체적인 이름을 사용해보면 좋을 것 같습니다 :)
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.
지금 당장 떠오르는 네이밍은 useCustomRouter
인데, 이런 느낌으로 커스텀훅이라는 것을 드러내면 좋을 것 같아요!
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.
이 부분에 대한 상세한 답변은 위에 남겨두었습니다.
실제로 Next.js에서도 path를 이동시키기 위해 useRouter라는 이름을 사용하고 있네요!
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.
좋습니다🎈🎈
import { useNavigate, useParams } from 'react-router-dom'; | ||
|
||
const useRouter = () => { | ||
const router = useNavigate(); |
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.
[P2]
현재 변수 router
에는 useNavigate
의 반환값을 담고 있네용! 변수명이 router
보다 navigate
가 더 적절한 것 같아요!
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.
동의합니다. navigate가 조금 더 명확한 표현인 것 같네요 😄
|
||
const useRouter = () => { | ||
const router = useNavigate(); | ||
const uuid = useParams<{ uuid: string }>().uuid; |
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.
[P3]
const uuid = useParams<{ uuid: string }>().uuid; | |
const { uuid } = useParams<{ uuid: string }>(); |
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.
[P2]
useRouter
훅을 만들어서 useNavigate
와 useParams
를 결합한 이유는 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;
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.
하지만 useNavigate는 이미 React Router에서 제공하는 네비게이션 함수로, 이걸 굳이 커스텀 훅으로 감싸서 사용할 필요가 있을지 의문이 듭니다. 단순히 경로를 변경하는 것은 navigate 내장 함수를 바로 사용하는 편이 더 명확할 것 같아요.
저는 빙봉의 의견과 조금 다릅니다..!!
지금까지 관습적으로 navigate
라는 변수명을 사용했기 때문에 navigate
라는 단어에 익숙해진 것이 아닐까요?
useNavigate를 사용하는 훅에서 사용하는 변수명을 제한시켜 동일한 코드를 제공하기 위함도 해당 useRouter의 역할이라고 생각합니다.
만약 다른 개발자가 useNavigate()
훅을 선언하는 변수명을 router
라고 설정할 수도 있기 때문이에요.
마지막으로, uuid는 우리 서비스에서 사용처가 많기 때문에 별도로 추가한 부가기능입니다. uuid가 없는 페이지에서도 사용할 수 있는 유연함을 제공하고 싶었어요.
또한, uuid가 있는 경로에서 해당 훅을 사용하더라도 라우팅 경로는 '/'
와 같이 원하는 곳으로 어디든 이동할 수 있다는 가정하에 선택해서 사용할 수 있도록 구현했습니다. 따라서 저는 useRouter라는 네이밍을 그대로 유지하고 싶은데 빙봉의 생각은 어떠하신가요?
만약 기존에 사용해왔던 navigate
변수를 동일하게 사용하고 싶다. 라고 하면 합의하에 routeTo
가 아닌, navigate
로 사용해볼 수 있을 것 같습니다!
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.
navigate라는 단어에 집중하기 보다는 경로를 변경하는 함수를 사용하는 방식이 현재 2가지라는 점에서 드렸던 코멘트입니다.
제가 이해한 바로는 낙타가 구현한 useRouter
훅에서 routeTo
함수는 useNavigate
를 통해 경로를 변경하는 역할을 하고 있는 것 같아요.
현재 코드에서 보면, useNavigate
를 사용하는 방식과 useRouter
의 routeTo
함수를 사용하는 방식이 서로 혼용될 수 있을 것 같습니다.
- 예시 1:
useNavigate
를 직접 사용하는 코드const navigate = useNavigate(); <button onClick={() => navigate("/")} />
- 예시 2:
useRouter
의routeTo
를 사용하는 코드const { routeTo } = useRouter(); <button onClick={() => routeTo(`/meeting/${uuid}`)} />
결국 두 방식은 동일한 기능을 제공하지만, 중복된 방식으로 네비게이션을 처리하는 문제가 발생할 수 있을 것 같아요. (현재 모모 코드에서도 예시 1번처럼 사용하는 경우도 있고, 예시 2번처럼 사용하는 경우도 있는 것 같네요.)
특히 useNavigate
는 React Router에서 이미 제공되는 함수인데, 이를 다시 커스텀 훅으로 감싸는 이유가 여전히 모호하게 느껴집니다.
저는 routeTo 함수가 useNavigate 함수 기능 외의 역할을 하고 있으면 남겨두는 것이 의미가 있지만, 그렇지 않다면 삭제하면 어떨까 하는 의견입니다.
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.
대 모 모 F E
우리는 코드 리뷰에 진심이다.
<> | ||
<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> | ||
</> |
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.
// 새로고침 시, 값을 받아오는 형태를 알 수 없으므로 null에 대한 처리: 기본값 설정 (@낙타) | ||
if (!currentMeetingType) return HttpResponse.json(meetingEntranceDateTimeInfo, { status: 200 }); |
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.
[P3]
굿👍 중복되는 코드가 있어서 아래처럼 코드 수정 제안드립니다~
// 새로고침 시, 값을 받아오는 형태를 알 수 없으므로 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 }); | |
} | |
}), | |
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.
고생하셨습니다! 낙타~
저희 서비스가 언제 이렇게 페이지들이 많아졌는지 모르겠는데, 헤더에 페이지별로 고민한 흔적이 인상깊네요. 특히 노션에 정리해 놓은 것을 보니 리뷰하기 편했습니다.
사소한 변경 사항을 요청드리려고 합니다~ 나중에 다시 봐용 ㅎ
@@ -15,5 +15,5 @@ export const s_toastListContainer = css` | |||
|
|||
width: 100%; | |||
max-width: 43rem; | |||
padding: 1.6rem; | |||
padding: 0 1.6rem; |
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.
[C]
ㅎㅎ..🤣
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.
[P1]
해당 tsx 파일명을 index.tsx
로 수정해주셔야 할 것 같습니다! 폴더, 파일명 컨벤션에 맞지 않는 것 같아요 현재는 ㅎㅎ
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.
감사용 해리티티~
const useRouter = () => { | ||
const router = useNavigate(); | ||
const uuid = useParams<{ uuid: string }>().uuid; | ||
|
||
return { | ||
uuid, | ||
routeTo: (path: string) => router(path), | ||
}; | ||
}; |
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.
[P3]
-
useRouter, useUuid 두 개로 커스텀 훅을 분리해보시는 것에 대해서 어떻게 생각하시나요? 관심사가 다른 커스텀 훅이라서, 분리를 하면 더 유연하게 사용할 수 있을 것 같아요. 예를 들어서, uuid만 알고 싶고 라우팅에 대한 책임은 없는 커스텀 훅이나 컴포넌트에서는 uuid를 알기 위해서 useRouter 훅을 호출해야할 것 같다는 생각이 들어서, 해당 방법을 추천드립니다!
-
useRouter 훅 내부에 router(-1)을 호출하는, goBack 함수를 추가로 만들어 보는 것은 어떨까요? 추상화 + 직관적인 느낌을 줄 수 있을 것 같아 의견을 제시해 봅니다!
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.
의견 제안 감사합니다 :)
useRouter, useUuid 두 개로 커스텀 훅을 분리해보시는 것에 대해서 어떻게 생각하시나요?
조금 더 추상화해본다면 해리의 의견이 정확히 맞는 것 같아요. 필요하면 선택적으로 사용할 수 있게 하기 위함의 목적으로 uuid를 선언한 것이 맞습니다. 하지만, 역시 두 관심사가 다르니 충분히 분리를 고려해볼만하네요!
useRouter 훅 내부에 router(-1)을 호출하는, goBack 함수를 추가로 만들어 보는 것은 어떨까요?
이 부분도 매우 동의합니다. 추가해볼게요~!
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.
[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 파일을 추가로 만들지 않고 색을 재사용할 수 있어요!
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.
![image](https://private-user-images.githubusercontent.com/106071687/378057121-4fd803e6-ed71-4ecf-b58f-b4cae471ce3d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk4MjIxMjYsIm5iZiI6MTczOTgyMTgyNiwicGF0aCI6Ii8xMDYwNzE2ODcvMzc4MDU3MTIxLTRmZDgwM2U2LWVkNzEtNGVjZi1iNThmLWI0Y2FlNDcxY2UzZC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxN1QxOTUwMjZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1kOWY3MjU1MWVkN2M5ZWU5OTczYjkyZjNlYTNhMjY2ZmM0ZjdjODhjY2M1ZGM0NDgxOWNhNTQ3MzliYjc1NzE2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.CArccL2jnL_IxIxN7IDen9NwH2PbN9IOy1JAI1MtGy4)
변경된 아이콘의 경우에는 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>
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.
그 방법도 좋네요!
<button css={s_backButton} onClick={() => routeTo(`/meeting/${uuid}`)}> | ||
<BackSVG width="24" height="24" /> | ||
</button> |
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.
[P2]
로그인 페이지가 입장 페이지에서만 사용된다면, 뒤로가기 버튼을 클릭했을 때,
routerTo(-1)
로 호출해 주면 되지 않을까요? useRouter 커스텀 훅 내부에서 뒤로가기 함수도 제공해 주면 될 것 같은데 어떻게 생각하시나요?
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.
저의 개인적인 의견으로 -1
을 사용하는 것은 많이 위험이 도사리고 있다고 생각합니다.
가장 큰 이유가 직접적으로 url에 접근한 경우를 생각해볼 수 있을 것 같은데요..! 그 때 뒤로가기를 눌렀을 때 개발자가 예상한대로 동작하지 않기 때문에 명시적인 경로로 기입해두었습니다!
<button css={s_backButton} onClick={() => navigate(-1)}> | ||
<BackSVG width="24" height="24" /> | ||
</button> |
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.
[P2]
<button css={s_backButton} onClick={() => navigate(-1)}>
<BackSVG width="24" height="24" />
</button>
->
<BackButton />
뒤로 이동 시키는 버튼 컴포넌트를 추상화해 보는 것은 어떨까요??
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.
그것도 굉장히 좋은 생각이네요 해리~ 추상화 해보겠습니다~!
해리는 추신(추상화의 신)이네요
// 새로고침 시, 값을 받아오는 형태를 알 수 없으므로 null에 대한 처리: 기본값 설정 (@낙타) | ||
if (!currentMeetingType) return HttpResponse.json(meetingEntranceDateTimeInfo, { status: 200 }); |
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.
리뷰 반영사항 반영하고 다시 요청합니다~!
변경된 자세한 사항은 노션 하단에 해빙 리뷰 후 반영사항 에서 확인할 수 있습니다.
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>
);
}
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.
![image](https://private-user-images.githubusercontent.com/106071687/378057121-4fd803e6-ed71-4ecf-b58f-b4cae471ce3d.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk4MjIxMjYsIm5iZiI6MTczOTgyMTgyNiwicGF0aCI6Ii8xMDYwNzE2ODcvMzc4MDU3MTIxLTRmZDgwM2U2LWVkNzEtNGVjZi1iNThmLWI0Y2FlNDcxY2UzZC5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE3JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxN1QxOTUwMjZaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1kOWY3MjU1MWVkN2M5ZWU5OTczYjkyZjNlYTNhMjY2ZmM0ZjdjODhjY2M1ZGM0NDgxOWNhNTQ3MzliYjc1NzE2JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.CArccL2jnL_IxIxN7IDen9NwH2PbN9IOy1JAI1MtGy4)
변경된 아이콘의 경우에는 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>
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.
하지만 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 = () => { |
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.
이 부분에 대한 상세한 답변은 위에 남겨두었습니다.
실제로 Next.js에서도 path를 이동시키기 위해 useRouter라는 이름을 사용하고 있네요!
import { useNavigate, useParams } from 'react-router-dom'; | ||
|
||
const useRouter = () => { | ||
const router = useNavigate(); |
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.
동의합니다. navigate가 조금 더 명확한 표현인 것 같네요 😄
|
||
const useRouter = () => { | ||
const router = useNavigate(); | ||
const uuid = useParams<{ uuid: string }>().uuid; |
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.
좋은 의견이에요 빙봉~! 🚀
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.
감사용 해리티티~
<button css={s_backButton} onClick={() => routeTo(`/meeting/${uuid}`)}> | ||
<BackSVG width="24" height="24" /> | ||
</button> |
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.
저의 개인적인 의견으로 -1
을 사용하는 것은 많이 위험이 도사리고 있다고 생각합니다.
가장 큰 이유가 직접적으로 url에 접근한 경우를 생각해볼 수 있을 것 같은데요..! 그 때 뒤로가기를 눌렀을 때 개발자가 예상한대로 동작하지 않기 때문에 명시적인 경로로 기입해두었습니다!
<button css={s_backButton} onClick={() => navigate(-1)}> | ||
<BackSVG width="24" height="24" /> | ||
</button> |
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.
그것도 굉장히 좋은 생각이네요 해리~ 추상화 해보겠습니다~!
해리는 추신(추상화의 신)이네요
@Yoonkyoungme 좋은 질문 남겨주셔서 감사합니다 :) [Q]
변경사항에 대한 유연한 대처가 어렵다고 생각이 들었습니다. 지금 당장 헤더에 필요한 기능은 뒤로가기, 공유하기 버튼만 존재합니다. 그렇다면 뒤로가기 버튼, 공유하기 버튼을 보여줄 지 고민하는 상태가 필요합니다.(ex. isShareButton, isBackButton) 하지만 나중에 기능 추가(예를 들어 특정 페이지에서 메뉴 네비게이션 바 버튼 기능 추가) 되었을 때 마다 해당 버튼을 보여줄지 말지 여부를 판단하는 상태가 필요하며, 해당 상태를 헤더에 props로 내려줘야 하는 상황이 생길 것 같았어요. |
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.
안정적인 뒤로가기 기능을 제공하기 위해서 열심히 코드를 작성하셨네요 낙타!
정말 고생하셨습니다~ 빙봉과 티키타카 코드 리뷰 정말 인상적이네요. 모모 서비스에서 중심이 되는 기능을 개발하는 PR에는 코멘트가 기본적으로 30개는 달리는 것 같네요...ㅋㅋ~
드디어 뒤로가기 기능이 생겼다니 백엔드 모모들이 정말 좋아할 것 같네요! 고생 많았어요~ 🐫
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.
대 모 모 F E
우리는 코드 리뷰에 진심이다.
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.
🐫 낙타 🐪 고생하셨습니다!!!!!! 코드 만 줄의 변경이라니~~~~ 엄청나군요
의견 교환하는 과정 재미있었습니다 ㅎㅅㅎ
다음은 웹 접근성 함께 작업하러 가시죠!
- 토스트 메시지의 padding 속성으로 입력부분이 가려지는 문제가 있어 위와 아래의 패딩을 0으로 설정
- @types/jest 설치 - storybook 업데이트
- 필요한 헤더에 뒤로가기 라우팅, 공유 기능 구현
기본 형태를 반환할 수 있도록 수정
231eac3
to
e160ed2
Compare
관련 이슈
작업 내용
각 페이지 별 헤더 사용처
→ 퍼널 패턴 이전 단계
특이 사항
리뷰 요구사항 (선택)