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] fix : ErrorPage과 App의 레이아웃(=DOM 구조)다른 오류 수정 #614

Merged
merged 4 commits into from
Sep 16, 2024

Conversation

BadaHertz52
Copy link
Contributor

@BadaHertz52 BadaHertz52 commented Sep 14, 2024


🚀 어떤 기능을 구현했나요 ?

수정이유

  • ErrorPage에 Footer가 빠져있었어요. ErrorPage과 App으l 레이아웃이 같고, 안의 내용만 달라야하는데 각각 따로 설정하다보니 이런 오류가 발생한 것 같아요.

🔥 어떻게 해결했나요 ?

  • ErrorPage과 App으l 레이아웃이 달라지는 오류를 잡기 위해서, PageLayout에서 Topbar,BreadCrumb,Main을 불러오고 children으로 Main의 하위 컴포넌트를 받을 수 있도록 했어요.
  • BreadCrumb 은 ErrorPage에서는 필요 없기 때문에 PageLayout에 BreadCrumb 사용 여부를 props로 받을 수 있도록 했어요.
  • 레이아웃을 손보면서 Topbar에 필요없어진 Sidebar, SidebarOpenButton 컴포넌트를 삭제했어.

📝 어떤 부분에 집중해서 리뷰해야 할까요?

📚 참고 자료, 할 말

  • Sidebar 잘 가 👋

- 삭제된 컴포넌트: Sidebar, SidebarOpenButtion
- Topbar, BreadCrumb, Main 추가
- children으로 필요한 Main의 하위 컴포넌트 받도록 함
- BreadCrumb 필요에 따라 화면에 보여지도록 props 추가
Copy link
Contributor

@ImxYJL ImxYJL left a comment

Choose a reason for hiding this comment

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

생각해보니 에러 페이지에서 topbar는 보이는데 footer는 안 보이는 게 이상하긴 하네요... 왜 그냥 넘겼지?!
#나도꼼꼼해지고싶다..


const ErrorPage = () => {
const { isSidebarHidden, isSidebarModalOpen, closeSidebar, openSidebar } = useSidebar();
Copy link
Contributor

Choose a reason for hiding this comment

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

레벨 3 초기에 사이드바 열심히 만들던 게 엊그제같은데...(비록 제가 만들진 않았지만...) 시간이 빠르네요 ㅠ_ㅠ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🥲그때 진짜 빨리 구현하느라고 ... 고생했죠

@@ -3,12 +3,12 @@ import { EssentialPropsWithChildren } from '@/types';
import * as S from './styles';

interface MainProps {
isBreadCrumb?: boolean;
isShowBreadCrumb?: boolean;
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

@soosoo22 soosoo22 left a comment

Choose a reason for hiding this comment

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

완전 초기에 만들었던 사이드바를 이제야 지우네요ㅎㅎㅎㅎ 안녕~ 사이드바~~
역시 필요없어진 컴포넌트는 그냥 두는 것이 아닌 바로 지우는 게 좋은 것 같아요!!

Copy link
Contributor

@chysis chysis left a comment

Choose a reason for hiding this comment

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

사이드바 수고했고... 바다도 고생 많았습니다 😎

Comment on lines +15 to +26
const PageLayout = ({ children, isNeedBreadCrumb = true }: EssentialPropsWithChildren<PageLayoutProps>) => {
const breadcrumbPathList = useBreadcrumbPaths();
const isShowBreadCrumb = isNeedBreadCrumb && breadcrumbPathList.length > 1;

return (
<S.Layout>
<S.Wrapper>{children}</S.Wrapper>
<S.Wrapper>
<Topbar />
{isShowBreadCrumb && <Breadcrumb pathList={breadcrumbPathList} />}
<Main isShowBreadCrumb={isShowBreadCrumb}>{children}</Main>
<Footer />
</S.Wrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

레이아웃 단에서 처리하는 것 좋네요👍

@BadaHertz52 BadaHertz52 merged commit 6b27483 into develop Sep 16, 2024
6 checks passed
@BadaHertz52 BadaHertz52 deleted the fe/fix/613_pageLayout branch September 24, 2024 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[FE] ErrorPage, App의 레이아웃 차이나는 오류를 수정합니다.
4 participants