[junhjeon] onboarding 02 #9
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
yoouyeon
reviewed
Oct 21, 2023
Comment on lines
+35
to
+50
| <Route | ||
| path="/user" | ||
| element={ | ||
| role !== 'guest' ? <UserPage /> : <Navigate to="/main" /> | ||
| } | ||
| /> | ||
| <Route | ||
| path="/manager" | ||
| element={ | ||
| role === 'mananger' || role === 'admin' ? ( | ||
| <ManagerPage /> | ||
| ) : ( | ||
| <Navigate to="/main" /> | ||
| ) | ||
| } | ||
| /> |
There was a problem hiding this comment.
요런 식으로 role을 비교하는 로직이 아래에서도 계속 반복되는데 이 부분을 상위 컴포넌트로 빼서 반복을 줄여주는 것도 좋을 것 같아요!
그리고 지금은 페이지들이 적어서 관리가 어렵지 않지만 페이지가 많아지거나 권한이 많아진다면 지금 방식대로라면 관리하기가 어려울 것 같아서 페이지 권한 정보를 객체로 관리한다거나, 아니면 적어도 같은 레벨의 권한을 가지고 있는 컴포넌트끼리 같은 상위 컴포넌트로 묶어주는 방법을 써 보는 것도 좋을 것 같네용
Comment on lines
14
to
17
| useEffect(() => { | ||
| const isLogin = localStorage.getItem('isLogin'); | ||
| setLogined(isLogin); | ||
| }); |
There was a problem hiding this comment.
특별히 매 랜더링마다 useEffect 훅을 실행해야 하는게 아니라면 dependency 배열을 추가해주는 것이 좋을 것 같습니다!!
Comment on lines
+34
to
41
| <div className="right"> | ||
| {role === 'admin' ? <Link to={'/admin'}>Admin</Link> : null} | ||
| {(role === 'manager' || role === 'admin') ? <Link to={'/manager'}>Manager</Link> : null} | ||
| {role === 'manager' || role === 'admin' ? ( | ||
| <Link to={'/manager'}>Manager</Link> | ||
| ) : null} | ||
| {role !== 'guest' ? <Link to={'/user'}>Users</Link> : null} | ||
| <Link to={'/main'}>Main</Link> | ||
| </div> |
There was a problem hiding this comment.
권한을 확인하는 부분이 App.tsx 파일이랑 navbar, sidebar에서도 반복되고 있어서 권한 확인하는 로직을 레이아웃보다 더 상위 컴포넌트에서 체크했으면 더 좋았을 것 같아요
Comment on lines
0
to
19
| .sidebar { | ||
| flex: 1; | ||
| width: 10px; | ||
| background-color: #333; | ||
| color: white; | ||
| padding: 20px; | ||
| } | ||
|
|
||
| .sidebar a { | ||
| text-decoration: none; | ||
| display: block; | ||
| color: white; | ||
| padding: 10px 0; | ||
| } | ||
|
|
||
| .sidebar a:hover { | ||
| background-color: #555; | ||
| } No newline at end of file | ||
| flex: 1; | ||
| width: 10px; | ||
| background-color: #333; | ||
| color: white; | ||
| padding: 20px; | ||
| } | ||
|
|
||
| .sidebar a { | ||
| text-decoration: none; | ||
| display: block; | ||
| color: white; | ||
| padding: 10px 0; | ||
| border: none; | ||
| } | ||
|
|
||
| .sidebar a:hover { | ||
| background-color: #555; | ||
| } |
There was a problem hiding this comment.
금요일날 리뷰 하면서도 말씀드렸지만 sass 문법은 중첩이 가능해서 아래처럼 쓸 수 있어요
.sidebar {
flex: 1;
width: 10px;
background-color: #333;
color: white;
padding: 20px;
a {
text-decoration: none;
display: block;
color: white;
padding: 10px 0;
&:hover{
background-color: #555;
}
}
}
이거 말고도 mixin이나 변수 설정, 내장 모듈에 편한 기능들이 많아서 https://sass-lang.com/documentation/ 요기 참고해보시는것도 좋을 것 같습니당
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.