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] api에러 등의 에러를 잡아 핸들링 #232

Merged
merged 41 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
b346179
chore: ErrorBoundary사용 시 에러가 뜨지 않도록 overlay 속성 끔
pakxe Aug 7, 2024
db2dffa
feat: 에러 상태를 전역적으로 관라하기 위한 훅과 컨텍스트 구현
pakxe Aug 7, 2024
743547f
feat: ErrorProvider가 ToastProvider를 감싸도록 작성
pakxe Aug 7, 2024
a5a826c
feat: api 콜할 때 이 useFetch훅을 사용해 전역상태로 만들어준 에러를 핸들하도록 구현
pakxe Aug 7, 2024
ec1a0e2
chore: 임시로 만들어둔 useFetch사용 예시
pakxe Aug 7, 2024
f0cd278
chore: useFetch의 request에 넘겨주기 위한 response 타입 export
pakxe Aug 7, 2024
714fc3e
feat: Toast를 수정해 사용하기 위해 임시로 가져옴
pakxe Aug 7, 2024
d4363ee
feat: 전역 에러를 옵저빙하면서 에러가 있다면 토스트를 띄우도록 기능 추가
pakxe Aug 7, 2024
bee6989
fix: 타입에 이름이 일치하지 않는 부분 수정
pakxe Aug 7, 2024
94fb5a8
feat: 서버 에러 code와 에러 메세지를 매칭하는 상수 구현
pakxe Aug 7, 2024
2ab518b
chore: msw, react-error-boundary 설치
pakxe Aug 7, 2024
050172a
chore: 브라우저 환경에서 msw를 사용하기 위한 세팅 파일 설치
pakxe Aug 7, 2024
88e628a
feat: msw를 앱 시작 지점에 연결
pakxe Aug 7, 2024
4af8344
test: 테스트를 위한 간단한 post 모킹 함수 구현
pakxe Aug 7, 2024
4608e68
feat: 핸들되지 않는 에러 또는 지정되지 않은 path로 이동시 보여줄 에러 페이지 구현
pakxe Aug 7, 2024
4d4ea35
feat: 불필요한 state제거와 type narrowing
pakxe Aug 7, 2024
eaf7584
feat: 다뤄지지 않는 에러가 발생했을 시 외부로 에러 던지기
pakxe Aug 7, 2024
92b3665
feat: 핸들되는 에러 판단을 위한 함수 구현
pakxe Aug 7, 2024
f002c57
feat: 불필요한 state, setTimeout제거
pakxe Aug 7, 2024
2652c2e
feat: 지정되지 않은 path로 이동 시 에러 페이지 띄우도록 함
pakxe Aug 7, 2024
c3bb543
feat: 반복되는 숫자 상수화와 불필요한 jsx 주석 제거
pakxe Aug 7, 2024
8431796
feat: 잡아서 다루지 않는 에러를 위한 에러 바운더리 컴포넌트 구현
pakxe Aug 7, 2024
7e0478f
chore: 불필요하게 사용되는 contentType관련 코드 제거
pakxe Aug 7, 2024
2b35ee0
feat: Toast에 showingTime을 전달해 애니메이션을 지속 시간만큼 유지할 수 있도록 함
pakxe Aug 7, 2024
fe2be35
feat: 에러를 일정 시간 후에 초기화하는 책임을 useError에 위임
pakxe Aug 7, 2024
252fa01
feat: 알 수 없는 에러에 대한 에러 메세지와 이름 작성
pakxe Aug 7, 2024
9593155
feat: 이벤트를 새로 생성하기 위한 api콜을 담당하는 useEvent훅 구현
pakxe Aug 7, 2024
dadc181
feat: useEvent를 사용해 이벤트를 생성하도록 수정
pakxe Aug 7, 2024
20eface
chore: 불필요한 import 제거
pakxe Aug 7, 2024
c65f6e7
chore: msw, react-error-boundary 라이브러리 설치
pakxe Aug 7, 2024
559ccc3
chore: 사용하지 않는 import와 주석 제거
pakxe Aug 7, 2024
a45f845
refactor: queryFn -> queryFuntion으로 프로퍼티명 변경
pakxe Aug 7, 2024
d12979a
design: Toast 디자인 수정
pakxe Aug 7, 2024
4130ecd
chore: msw를 사용하는 코드 주석 처리
pakxe Aug 7, 2024
892dbb0
chore: 린트 적용
pakxe Aug 7, 2024
eded4da
fix: /가 하나 더 들어가있던 부분 수정
pakxe Aug 7, 2024
322d21c
fix: 함수가 아닌 객체로 수정
pakxe Aug 7, 2024
5beb45a
design: px to rem
pakxe Aug 7, 2024
87aae32
refactor: 불필요한 useCallback제거
pakxe Aug 7, 2024
99747d5
chore: fadeIn -> fadeInWithTransformY 식으로 이름 변경
pakxe Aug 8, 2024
79cd93a
chore: 충돌 병합
pakxe Aug 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions client/src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,16 +1,23 @@
import {Outlet} from 'react-router-dom';
import {HDesignProvider, ToastProvider} from 'haengdong-design';
import {HDesignProvider} from 'haengdong-design';
import {Global} from '@emotion/react';

import {GlobalStyle} from './GlobalStyle';

Check failure on line 5 in client/src/App.tsx

View workflow job for this annotation

GitHub Actions / test

There should be at least one empty line between import groups
import {toastConfig} from 'react-simple-toasts';

Check failure on line 6 in client/src/App.tsx

View workflow job for this annotation

GitHub Actions / test

There should be at least one empty line between import groups

Check failure on line 6 in client/src/App.tsx

View workflow job for this annotation

GitHub Actions / test

`react-simple-toasts` import should occur before import of `./GlobalStyle`
Copy link
Contributor

Choose a reason for hiding this comment

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

혹시 react-simple-toasts는 어떤 것인가요??!!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 .. 토스트가 행동 라이브러리에 묶여있어서 조작이 불가능해 임시로 쓰려고 가져왔던건데요. 그냥 행동 라이브러리 토스트 폴더 그대로 복붙해와서 사용하는걸로 바꿔서 지금은 사용 안합니다! 패키지에선 지웠는데 코드엔 남아있었네요. 꼼꼼한 확인 고마워영~~!!!

import {ErrorProvider} from './ErrorProvider';

Check failure on line 7 in client/src/App.tsx

View workflow job for this annotation

GitHub Actions / test

There should be at least one empty line between import groups
import {ToastProvider} from '@components/Toast/ToastProvider';

Check failure on line 8 in client/src/App.tsx

View workflow job for this annotation

GitHub Actions / test

`@components/Toast/ToastProvider` import should occur before import of `./GlobalStyle`

toastConfig({theme: 'dark'});

const App: React.FC = () => {
return (
<HDesignProvider>
<Global styles={GlobalStyle} />
<ToastProvider>
<Outlet />
</ToastProvider>
<ErrorProvider>
<ToastProvider>
<Outlet />
</ToastProvider>
</ErrorProvider>
</HDesignProvider>
);
};
Expand Down
55 changes: 55 additions & 0 deletions client/src/ErrorProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import ERROR_MESSAGES from '@constants/errorMessage';

Check failure on line 1 in client/src/ErrorProvider.tsx

View workflow job for this annotation

GitHub Actions / test

There should be at least one empty line between import groups
import React, {createContext, useState, useContext, ReactNode} from 'react';

Check failure on line 2 in client/src/ErrorProvider.tsx

View workflow job for this annotation

GitHub Actions / test

`react` import should occur before import of `@constants/errorMessage`

// 에러 컨텍스트 생성
interface ErrorContextType {
hasError: boolean;
errorMessage: string;
setError: (error: ServerError) => void;
clearError: () => void;
}

const ErrorContext = createContext<ErrorContextType | undefined>(undefined);

// 에러 컨텍스트를 제공하는 프로바이더 컴포넌트
interface ErrorProviderProps {
children: ReactNode;
}

type ServerError = {
code: string;
message: string;
};

export const ErrorProvider: React.FC<ErrorProviderProps> = ({children}) => {
const [hasError, setHasError] = useState(false);
const [errorMessage, setErrorMessage] = useState('');

const setError = (error: ServerError) => {
setHasError(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

요것은 에러를 초기화해주는 역할인가요? true에서 false로 바꿔주기 위해

Copy link
Contributor Author

@pakxe pakxe Aug 7, 2024

Choose a reason for hiding this comment

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

저게 없으면 에러가 켜있는 시간에 다시 에러를 발생시킨 경우 후자의 에러가 무시됩니다.
예로 3초만에 토스트가 꺼진다고 한다면, 2.5초에서 다시 에러를 발생시킬 경우 3초의 토스트를 연장하는게 맞다고 생각해요. 근데 저 false로 만드는 라인이 없다면 다시 에러를 발생시켜도 처음 토스트의 3초를 채우면 바로 토스트가 사라지게 됩니다.

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

Choose a reason for hiding this comment

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

이 코멘트를 읽다가 갑자기 생각난 의견입니다.
첫번째 에러가 발생하고 첫번째 에러에 대한 ErrorToast가 닫히기 전에 두번째 에러가 발생합니다.
그럼 이 경우 두번째 에러로 ErrorToast가 변경 (혹은 시간 연장)이 아니라
첫번째 에러 토스트가 띄워지고 그 하단에 두번째 에러 토스트가 띄워지는건 어떨지 제안드립니다! (마치 스택 쌓듯이)

그렇다면 이에 대해서 에러 토스트가 페이지를 모두 덮을 정도로 무수히 많아진다면?이 떠오르는데요. 에러 토스트 스택에 높이 제한을 주는 것은 어떨지 생각이 드네용..

근데 이건 정말 제안이라서.. 무시하셔도 됩니댱

setErrorMessage('');

setTimeout(() => {
setHasError(true);
setErrorMessage(ERROR_MESSAGES[error.code] ?? '지금은 에러 코드가 안바뀌어서 에러 메세지가 없어요.');
}, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

setTimeout을 쓴 이유가 궁금해요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setter겹치면 제일 마지막 세터만 실행되는 줄 알았는데 setTimeout이 없어도 둘 다 실행되네요. 새로이 알아갑니다. 감사해요

};

const clearError = () => {
setHasError(false);
setErrorMessage('');
};

return (
<ErrorContext.Provider value={{hasError, errorMessage, setError, clearError}}>{children}</ErrorContext.Provider>
);
};

// 에러 컨텍스트를 사용하는 커스텀 훅
export const useError = (): ErrorContextType => {
const context = useContext(ErrorContext);
if (!context) {
throw new Error('useError must be used within an ErrorProvider');
}
return context;
};
2 changes: 1 addition & 1 deletion client/src/apis/request/event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ type RequestPostNewEvent = {
eventName: string;
};

type ResponsePostNewEvent = {
export type ResponsePostNewEvent = {
eventId: string;
};

Expand Down
35 changes: 35 additions & 0 deletions client/src/apis/useFetch.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import {useError} from '../ErrorProvider';

Check failure on line 1 in client/src/apis/useFetch.tsx

View workflow job for this annotation

GitHub Actions / test

There should be at least one empty line between import groups
import {useState} from 'react';

Check failure on line 2 in client/src/apis/useFetch.tsx

View workflow job for this annotation

GitHub Actions / test

`react` import should occur before import of `../ErrorProvider`

interface FetchOptions<T> {
queryFn: () => Promise<T>;
Copy link
Contributor

Choose a reason for hiding this comment

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

이름에 축약어를 사용하지 않는다.
// bad
const getBgColor = '#333333';
// good
const getBackgroundColor = '#333333';

단축어를 쓰지 말자는 컨벤션이 있었어요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

리액트쿼리와 유사해서 이름을 그대로 가져갔었어요. 지금은 queryFunction으로 바꿔보았습니다!!

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 useFetch = () => {
const {setError, clearError} = useError();
const [loading, setLoading] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

boolean 변수다 보니 isLoading, setIsLoading이 더 어울리지 않을까 싶어요!

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 [data, setData] = useState<any>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

a...n...y....

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 request = async <T,>({queryFn}: FetchOptions<T>): Promise<T | null> => {
setLoading(true);
clearError();
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://haengdong.pro/event/{eventId}/home 주소로 접속하면, eventName을 불러오는 요청과, eventLog를 불러오는 요청이 두개가 동시에 날라가게되는데요,
혹시 하나의 요청이 실패해서 error가 생긴 상태에서, 다른 하나의 요청이 가기 시작한다면 여기의 clearError로 인해 직전의 요청의 error가 사라지게 되는 걸까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

�고려해보진 않은 상황인데요.
이 페이지에서만 한정된 상황이지만, 두 api모두 eventId에만 종속되어있기 때문에 둘 다 오류날 경우 에러메세지는 똑같을 것으로 보여집니다.
다른 에러메세지가 보여질 수 있는 상황이라고 한다면, 그 여러 개의 메세지를 모두 보여주는게 괜찮을까요? 너무 지저분할 것 같단 생각에 하나만 띄우긴 했습니다.

무엇이 맞는진 잘 모르겠지만 여러 개의 api가 모두 에러가 나고 있다면 결국 두 api모두 유사한 이유이지 않을까요.. ? 네트워크라던지 인증이라던지, eventId가 올바르지 않는다던지..

어찌됐든 여러 개가 오류난다면 정상은 아니니 그냥 에러 바운더리로 보내버리는 것도 방법이겠습니다.....

정리하자면 제 생각안에서는 아래 방법들이 그나마 가능성 있어보이는데요.

  1. 토스트 다 띄우기
  2. 토스트 하나만 띄우기
  3. 토스트가 꺼지지 않은 상태에서 또 에러가 발생한다면 에러 바운더리로 보내버리기

토다리는 무엇이 더 적합한 방법이라고 생각하시나요? 이 방법 말고도 좋은게 있다면 지식을 나누어 주세요..!

Copy link
Contributor

Choose a reason for hiding this comment

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

제가 말한 상황은, 두가지 모두 오류가 나는 상황이 아니라 두가지 요청을 보내는 상황에 clearError()으로 인해서 이전의 에러가 지워지지 않는가 물어본거였습니다!

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 result = await queryFn();
setData(result);
return result;
} catch (error) {
if (error instanceof Error) {
const errorBody = await JSON.parse(error.message);
setError(errorBody);
console.log(error);
} else {
setError(new Error('An unknown error occurred'));
Copy link
Contributor

Choose a reason for hiding this comment

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

궁금해서 물어보는 건데, catch에서 잡는 error가 Error의 instance 가 아닌 경우도 있나요?

Copy link
Contributor

Choose a reason for hiding this comment

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

promise 같은 객체를 throw 해버리는 case도 있는 것 같은데, 만약 promise를 throw했다면 이 부분은 unkown error가 되는 것인가 싶어서...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

없을리 없다고 생각합니다. 에러를 개발자가 잡아서 명확한 이름으로 던지지 않는다면 그게 언노운에러니까요..!

물론 저 분기문은 타입 에러때문에 쓴 것입니다. 언노운 에러에는 message가 없을 수 있습니다.

프로미스를 던지는 경우는 잘 모르겠는데 throw new Promise.reject(new Error('...'))형태를 말씀하시는 걸까요? 이거라면 똑같은 에러로 취급됩니다.

아니라면 혹시 예시가 있을까요? 그리고 프로미스를 던져야하는 이유가 있을까요?! 이런 쪽으로는 생각해보지 못해서요. 좋은 아이디어라면 공유해주세용

Copy link
Contributor

Choose a reason for hiding this comment

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

간단한 예시로, Suspense의 내부동작에서 promise를 던지고, 이 결과를 잡아서 처리하는 것으로 알고 있습니당

Copy link
Contributor

Choose a reason for hiding this comment

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

예를들면 {status: 400, errorMessage: ~~~} 와 같이 던지는 경우같은걸 말하는거죠?
감사합니다~!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아 서스펜스가 던지는 에러가 프로미스 리젝트 에러라는 말씀이실까요?!?!

}
return null;
} finally {
setLoading(false);
}
};

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.

아마 미래에는 캐싱이 필요해질 수도 있을 것 같지만.. 지금은 너무 섣부를 것 같아 따라만 해봤습니다.

return {data, loading, request};
};
67 changes: 67 additions & 0 deletions client/src/components/Toast/Toast.style.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import {css, keyframes} from '@emotion/react';

Check failure on line 1 in client/src/components/Toast/Toast.style.ts

View workflow job for this annotation

GitHub Actions / test

There should be at least one empty line between import groups
Copy link
Contributor

Choose a reason for hiding this comment

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

emotion에 이런 기능이 있었군요~
전혀 몰랐네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저도몰랐습니다... 신피티...

import {ToastPosition} from './Toast.type';

type ToastMarginStyle = {
position?: ToastPosition;
bottom?: string;
top?: string;
};
Comment on lines +5 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

이런 부분들은 렌더링 상태와 엮어 useFixedLayout과 같은 hook으로 만들어 준다면, 다양한 곳에서 사용해 볼 수도 있겠네요!

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.

css layout을 바꿔주는 친구들인데
얘내를 hook으로 만든다면 동적으로 다양한 상황에서 layout을 바꿔줄 수 있다고 생각한거였어요~
적용해보라는것도 아니었고 그냥 문뜩 떠오른 거였어요
https://61a90feace7802003a4d9c45-ucidzxnkub.chromatic.com/?path=/story/hooks-useanchoredposition--centered-on-screen&globals=showSurroundingElements:false


// 애니메이션 키프레임 정의
const fadeIn = keyframes`
from {
opacity: 0;
transform: translateY(20px);
}
to {
opacity: 1;
transform: translateY(0);
}
`;

const fadeOut = keyframes`
from {
opacity: 1;
transform: translateY(0);
}
to {
opacity: 0;
transform: translateY(20px);
}
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

fadeIn / fadeOut 이름의 키프레임이, fade 작업만이 아니라, transform에 대한 변경사항도 포함하고 있으니, 키프레임 이름도 이를 포함하게 작성해 주는 것이 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어.. 혹시 추천하는 이름이 있을까요? fadeInWithTranstorm은 어떤가요........🥲

Copy link
Contributor

Choose a reason for hiding this comment

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

좋은 것 같은데요? FadeInWithTransformY 같은 ㄴ것도 좋을 것 같아용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

좋은 아이디어네용. 바로 반영했습니다~!!!!


export const toastMarginStyle = ({position, bottom, top}: ToastMarginStyle) =>
css({
position: 'absolute',
bottom: position === 'bottom' ? `${bottom}` : 'auto',
top: position === 'top' ? `${top}` : 'auto',
left: '50%',
transform: 'translate(-50%)',

width: '100%',
maxWidth: '48rem',
paddingInline: '1rem',
});

export const toastStyle = (isVisible: boolean) =>
css({
width: '100%',
padding: '0.625rem 1rem',

backgroundColor: 'gray',
boxShadow: '0 8px 12px rgba(0, 0, 0, 0.16);',
Copy link
Contributor

Choose a reason for hiding this comment

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

전부 rem 단위를 사용하고 있어서 rem으로 통일해서 작성해주는게 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

엌.... 이거 그러면 Hdesign도 수정해야할 것 같습니다. 일단 클라이언트에있는건 수정했어요!


borderRadius: '1.25rem',

// 애니메이션 추가
animation: `${isVisible ? fadeIn : fadeOut} 0.5s forwards`,
});

export const textStyle = () =>
css({
width: '100%',

color: 'white',

whiteSpace: 'pre-line',
});
Copy link
Contributor

Choose a reason for hiding this comment

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

prop을 받지 않는 css는 함수로 만들어주지 않아도 간단할 것 같아요~ 이름 자체가 get~~와 같은 함수형 네이밍이 아니기도 하고... ㅎㅎ

Suggested change
export const textStyle = () =>
css({
width: '100%',
color: 'white',
whiteSpace: 'pre-line',
});
export const textStyle =
css({
width: '100%',
color: 'white',
whiteSpace: 'pre-line',
});

79 changes: 79 additions & 0 deletions client/src/components/Toast/Toast.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/** @jsxImportSource @emotion/react */
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.

HDesign에서 그대로 복사해오느라 남아있었네요. 바로 지웠습니다! 꼼꼼한 확인 고마워요~~!!!

import {createPortal} from 'react-dom';
import {useState, useEffect} from 'react';
import {toastStyle, textStyle, toastMarginStyle} from './Toast.style';
import {ToastProps, ToastType} from './Toast.type';
import {Button, Flex, Icon, Text} from 'haengdong-design';

const renderIcon = (type: ToastType) => {
switch (type) {
case 'error':
return <Icon iconType="error" />;
case 'confirm':
return <Icon iconType="confirm" />;
case 'none':
return null;
default:
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Toast type은 error, confirm, none 세가지 밖에 없는데, 같은 null을 반환하는 none과 한번도 걸러지지 않을 default의 case를 분리한 이유가 있을까요?!

Copy link
Contributor Author

@pakxe pakxe Aug 7, 2024

Choose a reason for hiding this comment

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

Toast를 수정해서 사용하려고 복붙해오게 되었는데요. Hdesign의 Toast는 지노킴씨가 작업하셨으니 쿠키씨에게 여쭤볼게요!

Copy link
Contributor

Choose a reason for hiding this comment

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

지노킴이 대답할게요! 이거 개발환경 중에 그럴일은 없겠지만 error, confirm, none 이외의 다른 문자가 들어갔을 때 오류를 방지하기 위해 이렇게 했어요

Copy link
Contributor

Choose a reason for hiding this comment

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

개발환경이면 애초에 vsc가 걸러줄 것 같긴 한데, 확인했습니당~

Copy link
Contributor

Choose a reason for hiding this comment

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

'none'과 default의 return이 null로 동일하기 때문에 case 'none'에 대한 부분은 없어도 괜찮지 않을까 싶어요.
아니면 나중에 변경될 것을 고려하여 case를 남겨두신 걸까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아.. Hdesign에서 복붙해온거라 아마 거기에도 쓰여있을 거에요. 일단 client에서는 지웠습니다!

}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

아래와 같이 작성할 수도 있겠네요~!

Suggested change
const renderIcon = (type: ToastType) => {
switch (type) {
case 'error':
return <Icon iconType="error" />;
case 'confirm':
return <Icon iconType="confirm" />;
case 'none':
return null;
default:
return null;
}
};
const renderIcon = (type: ToastType) => {
if (type==='none') return null;
return <Icon iconType={IconType} />;
};

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 Toast = ({
type = 'confirm',
top = '0px',
bottom = '0px',
isClickToClose = true,
position = 'bottom',
message,
onUndo,
onClose,
...htmlProps
}: ToastProps) => {
const [isVisible, setIsVisible] = useState(true);
const styleProps = {position, top, bottom};

useEffect(() => {
const timer = setTimeout(() => {
setIsVisible(false);
setTimeout(() => {
if (onClose) onClose();
}, 500); // fadeOut 애니메이션 시간과 동일하게 설정
}, 3000); // 토스트가 화면에 보이는 시간 설정
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

@pakxe pakxe Aug 7, 2024

Choose a reason for hiding this comment

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

이게 어떤 말씀인지 잘 모르겠습니다.. 혹시 더 구체적으로 설명해주실 수 있나요? 일단 지금은 아래와 같이 바꿔두었습니다.

      }, ANIMATION_TIME); // fadeOut 애니메이션 시간과 동일하게 설정
    }, showingTime - ANIMATION_TIME); // 토스트가 내려가는 시간 확보

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

Choose a reason for hiding this comment

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

이런 시간 설정 매직넘버도 constants로 만들어도 좋을 것 같습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 바로 반영했습니다.


return () => {
clearTimeout(timer);
};
}, [onClose]);

const handleClickToClose = () => {
if (!isClickToClose || !onClose) return;

setIsVisible(false);
setTimeout(() => {
onClose();
}, 500); // fadeOut 애니메이션 시간과 동일하게 설정
};

return createPortal(
<div css={toastMarginStyle({...styleProps})} {...htmlProps} onClick={handleClickToClose}>
<div css={toastStyle(isVisible)}>
<Flex justifyContent="spaceBetween" alignItems="center">
<Flex alignItems="center" gap="0.5rem">
{renderIcon(type)}
<Text size="smallBodyBold" css={textStyle()}>
{message}
</Text>
</Flex>
{onUndo && (
<Button variants="tertiary" size="small" onClick={onUndo}>
되돌리기
</Button>
)}
</Flex>
</div>
</div>,
document.body,
);
};

export default Toast;
21 changes: 21 additions & 0 deletions client/src/components/Toast/Toast.type.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
export type ToastPosition = 'bottom' | 'top';
export type ToastType = 'error' | 'confirm' | 'none';

export interface ToastStyleProps {
bottom?: string;
top?: string;
}

export interface ToastOptionProps {
position?: ToastPosition;
type?: ToastType;
onUndo?: () => void;
isClickToClose?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

isClickToClose는 무엇을 하는 메소드 명인지 명확하지 않은 것 같아요.

  1. Close하기 위해 Click을 할 수 있는지 여부
  2. Close하기 위해 Click을 누른 상태
  3. Close될 때 Click이 발생하는 여부
    어느것이 맞는것인가요?

1번이라면, canClickToClose
2번이라면 isClickedToClose
3번이라면 isClickedWhenClose
와 같이 작성한다면 좀 더 직관적이지 않을까 싶어요!

Copy link
Contributor

Choose a reason for hiding this comment

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

boolean을 나타낼 떄 is를 붙이라는게 모든 상황에서 is를 붙이라는건 아닌걸로 알고 있어요.
is를 붙이는 경우는 형용사와 같이 상태를 수식하는 말일때 쓰는 것으로 알고 있어요!

  • is + Opened (인가? + 열려있는(상턔))
  • is + Valid (인가? + 유효한(상턔))
  • is + Filled (인가? + 가득찬(상턔))
  • is + Empty (인가? + 비어있는(상턔))

상태를 나타내는 형용사가 아닌 동사나 명사형을 쓰는경우라면, can, have 등이 가능할 것 같네요.

  • has + State (있는가? + 상태)
  • has + Value (있는가? + 값)
  • has + Cursor (있는가? + 커서)
  • has + Data (있는가? + 데이터)
  • can + Scroll (가능한가? + 스크롤하다)
  • can + Click (가능한가? + 클릭하다)
  • can + Request (가능한가? + 요청하다)
  • can + View (가능한가? + 보다)

정 is를 쓰고 싶다면,

  • canScroll -> isAbleToScroll
  • canClick -> isAbleToClick
  • canRequest -> isAbleToRequest
  • hasState -> isHavingState
  • hasValue -> isHavingValue
  • hasData -> isHavingData

정도로 바꿔쓰면 어떨까 싶어요!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

이것도 마찬가지로 라이브러리에서 긁어온것인데요. 아마 토스트사용하는 방식을 좀 바꿀 것 같습니다. 프로바이더를 사용하지 않는 방식으로요!

그래서 이슈를 만들어 두었는데 이 곳에 할 일로 적어뒀습니다.

onClose?: () => void;
}

export interface ToastRequiredProps {
message: string;
}

export type ToastProps = React.ComponentProps<'div'> & ToastStyleProps & ToastOptionProps & ToastRequiredProps;
82 changes: 82 additions & 0 deletions client/src/components/Toast/ToastProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/** @jsxImportSource @emotion/react */
import {createContext, useCallback, useContext, useEffect, useState} from 'react';
import {ToastProps} from './Toast.type';
import Toast from './Toast';
import {useError} from '../../ErrorProvider';

export const ToastContext = createContext<ToastContextProps | null>(null);

interface ToastContextProps {
showToast: (args: ShowToast) => void;
}

type ShowToast = ToastProps & {
showingTime?: number;
isAlwaysOn?: boolean;
};

const ToastProvider = ({children}: React.PropsWithChildren) => {
const [currentToast, setCurrentToast] = useState<ShowToast | null>(null);
const {hasError, errorMessage, clearError} = useError();

const showToast = useCallback(({showingTime = 3000, isAlwaysOn = false, ...toastProps}: ShowToast) => {
setCurrentToast({showingTime, isAlwaysOn, ...toastProps});
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

toast에 영향을 미치는 prop들을 useCallback의 dependancy array에 추가해서 값이 변경되면 memo된 값을 변경시켜주지 않아도 되나요?!

Copy link
Contributor Author

@pakxe pakxe Aug 7, 2024

Choose a reason for hiding this comment

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

useCallback아마 지피티한테 밥줘서 생긴 것 같습니다. 사용할 의도가 전혀 없었어서 지금은 지웠습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

밥줘서 ㅋㅋㅋㅋㅋㅋㅋㅋㅋ


const closeToast = () => {
setCurrentToast(null);
};

useEffect(() => {
if (hasError) {
showToast({
message: errorMessage || 'An error occurred',
showingTime: 5000, // 필요에 따라 시간 설정
isAlwaysOn: false,
position: 'bottom',
bottom: '100px',
Copy link
Contributor

Choose a reason for hiding this comment

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

마찬가지로 rem단위면 좋을 것 같습니다~!

});

const timer = setTimeout(() => {
clearError();
}, 5000); // 토스트가 표시되는 시간과 동일하게 설정

return () => clearTimeout(timer);
}

return;
}, [hasError, errorMessage, showToast, clearError]);

useEffect(() => {
if (!currentToast) return;

if (!currentToast.isAlwaysOn) {
const timer = setTimeout(() => {
setCurrentToast(null);
}, currentToast.showingTime);

return () => clearTimeout(timer);
}

return;
}, [currentToast]);
Copy link
Contributor

Choose a reason for hiding this comment

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

여기 useEffect를 나누어서 두 번 적은 이유가 궁금합니다~
hasError일 때 showToast를 통해서 토스트가 실행되고 타이머가 정해진 시간만큼 보여지게 되는데 아래 currentToast가 있을 때 다시 실행되는 것 같아서? (사실 정확하게는 몰라서 아닐수도?)

그리고 나중 이야기지만 Provider에 isAlwaysOn, showingTime 설정할 수 있도록 열어줘도 좋지 않을까 살짝 의견!?

Copy link
Contributor Author

@pakxe pakxe Aug 7, 2024

Choose a reason for hiding this comment

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

위에는 에러에 대한 이펙트고 아래는 이미 있던 이펙트같습니다. 위에만 새로 추가한것이에요.
위는 구현을 빨리 하기위해 대충 돌아가는 코드만 써두었습니다. 반복되는 로직을 묶기엔 시간부족이슈가 있었어요.

Provider에 isAlwaysOn, showingTime 설정할 수 있도록 열어줘도

혹시 위 말이 어떤 것을 의미하는걸까요? 뭘 위해서, 어떻게 사용하길 원하는지 잘 상상이 안됩니다 ㅜ..

Copy link
Contributor

Choose a reason for hiding this comment

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

showingTime이랑 isAlwaysOn이 지금은 5초랑 false로 고정이 되어있는데, 이를 변경하고 싶다고 할 때 외부에서 값을 넣어주면 조정하기 쉬울 것 같아서. 마치 아래 queryClient의 staleTime처럼
외부에서 값을 넣어주면 그것대로 작동하는 방식!

const queryClient = new QueryClient({ defaultOptions:
    { queries: 
        { staleTime: Infinity,
         }, 
     }, 
   })

Copy link
Contributor

Choose a reason for hiding this comment

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

provider의 value로 넣어서 children에서 useToast로 사용할 수 있게 하자는 얘기이지 않나 싶어요~


return (
<ToastContext.Provider value={{showToast}}>
{currentToast && <Toast onClose={closeToast} {...currentToast} />}
{children}
</ToastContext.Provider>
);
};

const useToast = () => {
const context = useContext(ToastContext);

if (!context) {
throw new Error('useToast는 ToastProvider 내에서 사용되어야 합니다.');
}

return context;
};

export {ToastProvider, useToast};
Loading
Loading