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

Feat/온보딩 2주차 #1 #6

Open
wants to merge 54 commits into
base: main
Choose a base branch
from

Conversation

Clearsu
Copy link

@Clearsu Clearsu commented Oct 20, 2023

2주차 결과물입니다.

  • 일반 유저
    id : jincpark
    password : 1q2w3e4r

  • 매니저
    id: banana
    password: bananaisyellow

  • 관리자
    id: apple
    password: appleisred

Copy link
Member

@yoouyeon yoouyeon left a comment

Choose a reason for hiding this comment

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

코드리뷰 전에 리뷰 일부 남겨봤는데 나머지는 직접 질문 드릴게요! 수고하셨습니다~!! 👏👏👏

import { React } from 'react';
import NavBar from './NavBar';
import SideBar from './SideBar';
import PropTypes from 'prop-types';
Copy link
Member

Choose a reason for hiding this comment

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

PropTypes 이용해서 js에서도 타입체킹을 할 수 있네요..! 배워갑니다 👍

Copy link
Author

Choose a reason for hiding this comment

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

@yoouyeon eslint가 강제하는 바람에 하게 됬네요ㅋㅋ

Comment on lines +11 to +13
if (!userStateValue.isLoggedIn && normal) {
return <Navigate to="/login" replace={true} />;
}
Copy link
Member

Choose a reason for hiding this comment

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

의도하신건지는 모르겠지만 로그인 상태가 아닐 때 /manage 같은 normal 권한이 아닌 페이지에 접근했을 때에는 로그인 페이지가 아니라 아래 14번째 줄의 조건문으로 들어가네요!
로그인 상태가 아닐 때 normal, manager, admin 권한 중 하나라도 있으면 로그인 페이지로 이동시키는 방식으로 한다면 로그아웃 상태의 유저를 통일성있게 처리할 수 있을 것 같습니당

Copy link
Author

Choose a reason for hiding this comment

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

@yoouyeon 의도한 것이긴 합니다! 이렇게 하는게 보안상의 의미가 있는지는 모르겠지만, 외부에서 매니저 페이지 혹은 관리자 페이지의 url을 알지 못 하게 하고 싶었습니다😅

import userState from '../userState';
import PageNotFound from '../pages/PageNotFound';

export default function ProtectedRoute({ normal, manager, admin }) {
Copy link
Member

Choose a reason for hiding this comment

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

이 부분에서 prop이름이 로그인된 유저의 권한인지, 페이지인지 헷갈려서 normalPage 이런 식으로 좀 더 명확하게 해주면 좋을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

@hyobb109 반영하도록 하겠습니다 감사합니다 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants