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] feat: 에러, 로딩 페이지 구현 및 적용 #155

Merged
merged 11 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
3 changes: 3 additions & 0 deletions frontend/src/assets/alertTriangle.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions frontend/src/assets/home.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
3 changes: 3 additions & 0 deletions frontend/src/assets/reload.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions frontend/src/components/ReviewPreviewCard/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export const Visibility = styled.div`
display: flex;
gap: 0.6rem;
align-items: center;

font-size: 1.6rem;
font-weight: 700;

Expand Down
10 changes: 8 additions & 2 deletions frontend/src/components/common/Button/index.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

버튼에 텍스트 뿐 아니라 아이콘까지 추가돼서 이와 같이 코드를 작성하신 것 같아요. 추후에 텍스트, 아이콘 이외의 요소들이 더 들어올 상황이 생길 수도 있을 것 같아요. 이런 경우에 optional로 props를 계속 받기보다는, 공통 버튼 컴포넌트는 Children만을 받아서 그대로 출력해주는 것은 어떨까요?

그렇게 되면 버튼을 사용하는 쪽에서 필요한 요소들을 유연하게 넣을 수 있고, 현재 코드에서의 icon도 그 종류가 특정될 수 있을 것 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

너무 동의합니다!
에프이가 말했던 것처럼 추가적으로 요소들이 더 들어오면 아래 코드처럼 옵셔널로 props를 받는 상황이 많아질텐데.... children을 받는 방법이 더 효율적일 것 같네요! 고마워요~~😊

// 안 좋은 코드
interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> {
  buttonType: ButtonType;
  text: string;
  image?: string;
  imageDescription?: string;
}

Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@ import * as S from './styles';
interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> {
buttonType: ButtonType;
text: string;
icon?: string;
}

const Button = ({ buttonType, text }: ButtonProps) => {
return <S.Button buttonType={buttonType}>{text}</S.Button>;
const Button = ({ buttonType, text, icon, onClick }: ButtonProps) => {
return (
<S.Button buttonType={buttonType} onClick={onClick}>
{icon && <S.Icon src={icon} alt="아이콘" />}
Copy link
Contributor

Choose a reason for hiding this comment

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

alt는 사진에 대한 자세한 설명이여야해요.
alt을 특정 아이콘에 대한 내용으로 하고 싶다면 icon 내용을 props로 받을 수 있고,
특정 하기 힘들다면 빈문자열을 사용하는 것을 추천드려요

Copy link
Contributor

Choose a reason for hiding this comment

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

바다 말대로 alt를 props로 받으면 좋을 것 같습니다. 별도로 alt를 전달하지 않으면 빈 문자열('')을 주는 방식으로 개선해보면 좋을 것 같아요!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

너무나 동의하는 바입니다! 제가 작성한 코드인데도... alt="아이콘" 뭘 설명하는지 모르겠네요...^^

{text}
</S.Button>
);
};

export default Button;
4 changes: 4 additions & 0 deletions frontend/src/components/common/Button/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,7 @@ export const Button = styled.button<{ buttonType: ButtonType }>`

${({ buttonType, theme }) => getButtonStyle(buttonType, theme)};
`;

export const Icon = styled.img`
margin-right: 8px;
Copy link
Contributor

@BadaHertz52 BadaHertz52 Jul 31, 2024

Choose a reason for hiding this comment

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

px??? 우리는 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.

px을 좋아하나봐요....^^
바로~~ 고칠게요~~ 예리하세요~~

`;
2 changes: 2 additions & 0 deletions frontend/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { createBrowserRouter, RouterProvider } from 'react-router-dom';
import App from '@/App';

import DetailedReviewPage from './pages/DetailedReviewPage';
import ErrorPage from './pages/ErrorPage';
import ReviewPreviewListPage from './pages/ReviewPreviewListPage';
import ReviewWritingPage from './pages/ReviewWriting';
import globalStyles from './styles/globalStyles';
Expand All @@ -15,6 +16,7 @@ const router = createBrowserRouter([
{
path: '/',
element: <App />,
errorElement: <ErrorPage />,
children: [
{
path: 'user',
Expand Down
8 changes: 5 additions & 3 deletions frontend/src/pages/DetailedReviewPage/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { ReviewComment } from '@/components';
import { DetailReviewData } from '@/types';

import { getDetailedReviewApi } from '../../apis/review';
import LoadingPage from '../LoadingPage';
Copy link
Contributor

Choose a reason for hiding this comment

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

리뷰 상세페이지에 LoadingPage를 적용시켰군요!
develop에 머지 후 리뷰 상세페이지의 suspense에 적용해볼게요. 😝


import KeywordSection from './components/KeywordSection';
import ReviewDescription from './components/ReviewDescription';
Expand All @@ -18,14 +19,15 @@ const DetailedReviewPage = () => {
const memberId = queryParams.get('memberId');

const [detailedReview, setDetailReview] = useState<DetailReviewData>();
const [isLoading, setIsLoading] = useState(false);
const [isLoading, setIsLoading] = useState<boolean>(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

초기값을 false로 지정해주고 있어서 타입 추론이 자동으로 이루어질 것 같아요. boolean을 명시한 쑤쑤만의 이유가 있을까요?

Copy link
Contributor Author

@soosoo22 soosoo22 Jul 31, 2024

Choose a reason for hiding this comment

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

isLoading에 타입을 boolean으로 더 명확하게 지정하고 싶었던 것 같아요

const [errorMessage, setErrorMessage] = useState<string>('');

useEffect(() => {
const fetchReview = async () => {
try {
const result = await getDetailedReviewApi({ reviewId: Number(id), memberId: Number(memberId) });
setIsLoading(true);

const result = await getDetailedReviewApi({ reviewId: Number(id), memberId: Number(memberId) });
setDetailReview(result);
setErrorMessage('');
} catch (error) {
Expand All @@ -38,7 +40,7 @@ const DetailedReviewPage = () => {
fetchReview();
}, [id]);

if (isLoading) return <div>Loading...</div>;
if (isLoading) return <LoadingPage />;

if (errorMessage) return <div>Error: {errorMessage}</div>;
if (!detailedReview) return <div>Error: 상세보기 리뷰 데이터를 가져올 수 없어요.</div>;
Expand Down
33 changes: 33 additions & 0 deletions frontend/src/pages/ErrorPage/components/ErrorSection/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import AlertTriangle from '@/assets/alertTriangle.svg';
import Home from '@/assets/home.svg';
import ReLoad from '@/assets/reload.svg';
Copy link
Contributor

Choose a reason for hiding this comment

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

svg를 import 할때 Icon을 뒤에 붙이기로 했어요.

import HomeIcon from '@/assets/home.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.

어머어머 고마워요~
코드컨벤션에도 기재되어 있는 내용인데.... 간과했네요!

import { Button } from '@/components';

import * as S from './styles';

interface ErrorSectionProps {
errorMessage: string;
handleReLoad: () => void;
handleGoHome: () => void;
}

const ErrorSection = ({ errorMessage, handleReLoad, handleGoHome }: ErrorSectionProps) => {
return (
<S.Layout>
<S.ErrorLogoWrapper>
<img src={AlertTriangle} alt="에러 로고" />
</S.ErrorLogoWrapper>
<S.ErrorMessage>{errorMessage}</S.ErrorMessage>
<S.Container>
<S.ButtonContainer>
<Button buttonType="primary" text="새로고침하기" icon={ReLoad} onClick={handleReLoad} />
</S.ButtonContainer>
<S.ButtonContainer>
<Button buttonType="secondary" text="홈으로 이동하기" icon={Home} onClick={handleGoHome} />
</S.ButtonContainer>
Copy link
Contributor

Choose a reason for hiding this comment

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

ButtonContainer 부분이 반복되어서, 버튼의 icon,text,buttonType,click을 가지는 객체가 요소인 배열을 만들어서 활용해봐도 좋을 것 같아요.
click 핸들러에서 버튼의 name에 따라 다른 함수를 실행시킬 수 있도록 해도 되고요.

하나의 제안일 뿐이니 반영해야한다는 의견은 아니에요

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.

반복되는 코드들이 많아서 객체 배열을 만들고 map 을 이용해서 렌더링하는 게 더 효율적이겠네요~
고마워요~~😘

  const buttons = [
    {
      buttonType: 'primary' as ButtonType,
      text: '새로고침하기',
      image: ReloadIcon,
      imageDescription: '새로고침 이미지',
      onClick: handleReload,
    },
    {
      buttonType: 'secondary' as ButtonType,
      text: '홈으로 이동하기',
      image: HomeIcon,
      imageDescription: '홈 이미지',
      onClick: handleGoHome,
    },
  ];

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.

아! 지금보니 쓸모없는 컨테이너네요!
아이콘이 버튼 안에 들어가야 하는데 처음에는 하나의 컨테이너에 아이콘과 버튼 이렇게 나란히 둬서 그랬던 것 같아요

</S.Container>
</S.Layout>
);
};

export default ErrorSection;
42 changes: 42 additions & 0 deletions frontend/src/pages/ErrorPage/components/ErrorSection/styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import styled from '@emotion/styled';

export const Layout = styled.div`
display: flex;
flex-direction: column;
gap: 3rem;
align-items: center;
justify-content: center;

height: calc(100vh - 21rem);
Copy link
Contributor

Choose a reason for hiding this comment

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

21rem이 무엇에서 왔는지 알 수 있을까요?

`;

export const ErrorLogoWrapper = styled.div`
& > img {
width: 15rem;
height: 15rem;
}
`;

export const ErrorMessage = styled.span`
font-size: 2rem;
`;

export const Container = styled.div`
display: flex;
gap: 3.5rem;
align-items: center;
justify-content: center;
`;

export const ButtonContainer = styled.div`
display: flex;
align-items: center;
justify-content: center;
margin-top: 3rem;

& > button {
width: 17rem;
height: 5rem;
font-size: 1.4rem;
}
`;
43 changes: 43 additions & 0 deletions frontend/src/pages/ErrorPage/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { useNavigate } from 'react-router';

import { Main, PageLayout, SideModal, Sidebar, Topbar } from '@/components';
import { useSidebar } from '@/hooks';

import ErrorSection from './components/ErrorSection';

const ERROR_MESSAGE = {
server_unstable: '서버와의 통신이 불안정합니다.',
Copy link
Contributor

Choose a reason for hiding this comment

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

key에 카멜이 아니라 스네이크 케이스를 쓴 이유가 있을까요?
이전 상수 변수들의 키값 네이밍 형식과는 안맞아서요.

Copy link
Contributor

Choose a reason for hiding this comment

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

ErrorPage가 route의 errorElement에 사용되는데 이 경우는 서버와의 통신 불안이 아닌, 찾는 페이지가 없는 오류 아닐까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

key에 카멜이 아니라 스네이크 케이스를 쓴 이유가 있을까요? 이전 상수 변수들의 키값 네이밍 형식과는 안맞아서요.

어머어머... 이런... 저는.. 코드 컨벤션도 지키지 못하는 팀원이네요😅 수정할게요~~
커밋 주소 : 930b117

};

const ErrorPage = () => {
const { isSidebarHidden, isSidebarModalOpen, closeSidebar, openSidebar } = useSidebar();
const navigate = useNavigate();

const handleReLoad = () => {
window.location.reload();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

useNavigate에서 제공하는 navigate(0)를 사용하면 현재 페이지 새로고침이 가능해요. navigate를 사용하고 있어서 제안드려요😊

Copy link
Contributor

Choose a reason for hiding this comment

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

사소한 부분이지만😅, reload를 하나의 단어로 보지 않고 ReLoad로 표기한 이유가 있을까요?

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에서 제공하는 navigate(0)를 사용하면 현재 페이지 새로고침이 가능해요. navigate를 사용하고 있어서 제안드려요😊

아하! 고마워요! 이미 홈으로 이동할 때, navigate를 사용하고 있어서 새로고침도 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.

사소한 부분이지만😅, reload를 하나의 단어로 보지 않고 ReLoad로 표기한 이유가 있을까요?

아하.....re+load 약간 이런 느낌이라고 생각해서 ReLoad라고 표기했던 것 같아요😭 수정할게요~


const handleGoHome = () => {
navigate('/'); // TODO: 홈 페이지 경로가 결정되면 변경 필요
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO 주석 좋아요!

};

return (
<PageLayout>
{isSidebarModalOpen && (
<SideModal isSidebarHidden={isSidebarHidden}>
<Sidebar closeSidebar={closeSidebar} />
</SideModal>
)}
<Topbar openSidebar={openSidebar} />
<Main>
<ErrorSection
errorMessage={ERROR_MESSAGE.server_unstable}
handleReLoad={handleReLoad}
handleGoHome={handleGoHome}
/>
</Main>
</PageLayout>
);
};

export default ErrorPage;
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import * as S from './styles';

const LoadingBar = () => {
return <S.LoadingBar />;
};

export default LoadingBar;
41 changes: 41 additions & 0 deletions frontend/src/pages/LoadingPage/components/LoadingBar/styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import styled from '@emotion/styled';

export const LoadingBar = styled.div`
position: relative;

display: block;

width: 30rem;
height: 2rem;

background-color: ${({ theme }) => theme.colors.lightPurple};
border-radius: 3rem;

::before {
content: '';

position: absolute;
top: 0;
left: 0;

width: 0%;
height: 100%;

background: ${({ theme }) => theme.colors.primary};
border-radius: 3rem;

animation: moving 1s ease-in-out infinite;
}

@keyframes moving {
50% {
width: 100%;
}

100% {
right: 0;
left: unset;
width: 0;
}
}
`;
13 changes: 13 additions & 0 deletions frontend/src/pages/LoadingPage/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import LoadingBar from './components/LoadingBar';
import * as S from './styles';

const LoadingPage = () => {
return (
<S.Container>
<S.Text>Loading ....</S.Text>
<LoadingBar />
</S.Container>
);
};

export default LoadingPage;
17 changes: 17 additions & 0 deletions frontend/src/pages/LoadingPage/styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import styled from '@emotion/styled';

export const Container = styled.div`
display: flex;
flex-direction: column;
gap: 1.5rem;
align-items: center;
justify-content: center;

width: 100%;
height: calc(100vh - 21rem);
`;

export const Text = styled.span`
font-size: ${({ theme }) => theme.fontSize.basic};
color: ${({ theme }) => theme.colors.primary};
`;
4 changes: 3 additions & 1 deletion frontend/src/pages/ReviewPreviewListPage/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import { getReviewListApi } from '@/apis/review';
import ReviewPreviewCard from '@/components/ReviewPreviewCard';
import { ReviewPreview } from '@/types';

import LoadingPage from '../LoadingPage';

import SearchSection from './components/SearchSection';
import * as S from './styles';

Expand Down Expand Up @@ -44,7 +46,7 @@ const ReviewPreviewListPage = () => {
<S.Layout>
<SearchSection onChange={() => {}} options={OPTIONS} placeholder={USER_SEARCH_PLACE_HOLDER} />
<S.ReviewSection>
{loading && <p>로딩 중...</p>}
{loading && <LoadingPage />}
{error && <p>{error}</p>}
{!loading &&
!error &&
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/pages/ReviewWriting/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import Button from '@/components/common/Button';
import { REVIEW } from '@/constants/review';
import { Keyword, ReviewContent, ReviewData, WritingReviewInfoData } from '@/types';

import LoadingPage from '../LoadingPage';

import KeywordButton from './components/KeywordButton';
import RevieweeComment from './components/RevieweeComment';
import ReviewItem from './components/ReviewItem';
Expand Down Expand Up @@ -74,7 +76,7 @@ const ReviewWritingPage = () => {
}
};

if (!dataToWrite) return <div>Loading...</div>;
if (!dataToWrite) return <LoadingPage />;

return (
<S.ReviewWritingPage onSubmit={handleSubmitReview}>
Expand Down