부산대_FE_이민경_2주차 과제 Step2 #48
Conversation
| ); | ||
| } | ||
|
|
||
| export default AppRoutes; |
There was a problem hiding this comment.
지금 파일/폴더 등에 대한 일관성이 전혀 없어요..!
routers 폴더에서 내보내는 index.tsx에 있는 AppRoutes를 Router라는 이름으로 받아와서 사용하고 있습니다.
민경님이 아닌 다른 사람이 이런 구조/흐름으로 된 코드를 봤을 때 무척 헷갈리지 않을까 싶어요!
App.tsx에서 import { Router } from './routes'; 이런 형태로 import 하고, 내부에서도 Router 라는 이름으로 export 해주면 어떨까 싶네요!
아니면 반대로 AppRouter 로 export 하고, 가져와서 사용할 때에도 AppRouter 로 사용하는거죠 ㅎㅎ
코드를 작성할 때에는 항상 일관성이 제일 중요하답니다!
| return ( | ||
| <AuthProvider> | ||
| <Router> | ||
| <Header isLoggedIn={isLoggedIn}/> | ||
| <Routes> | ||
| <Route path="/" element={<Home />} /> | ||
| <Route path="/theme/:themeKey" element={<Theme />} /> | ||
| <Route path="/login" element={<Login onLogin={handleLogin}/>} /> | ||
| <Route path="/my-account" element={<PrivateRoute><MyAccount username={user} onLogout={handleLogout}/></PrivateRoute>} /> | ||
| </Routes> | ||
| <Footer /> | ||
| </Router> | ||
| </AuthProvider> | ||
| ); |
| const handleLogin = (username:string) => { | ||
| setuser(username); | ||
| setIsLoggedIn(true); | ||
| }; | ||
|
|
||
| const handleLogout = () => { | ||
| setuser(''); | ||
| setIsLoggedIn(false); | ||
| }; |
There was a problem hiding this comment.
네이밍에 대해서도 고찰이 필요할 것 같아요.
handleLogin 의 경우 login 이벤트가 발생했을 때 처리하는 이벤트라고 볼 수 있는데,
이 코드의 흐름으로 봤을 때 로그인이 발생했을 때 처리하는 이벤트가 아니라 "로그인 처리를 하는 함수"로 쓰이고 있습니다.
그렇다면 이 함수의 이름은 login이 되어야 하지 않을까요?
| <Routes> | ||
| <Route path="/" element={<Home />} /> | ||
| <Route path="/theme/:themeKey" element={<Theme />} /> | ||
| <Route path="/login" element={<Login onLogin={handleLogin}/>} /> |
There was a problem hiding this comment.
앞선 리뷰를 토대로 이벤트에 대해 고찰이 필요합니다.
onLogin => 로그인이라는 행위가 발생했을 때 실행하는 이벤트
login => 로그인을 실행하는 함수
이벤트를 지금 잘못쓰고 있지 않나요~?
이 경우에는 로그인 로직이 어디에 있어야 좋을지 고민이 필요해요!
| import Footer from '@/components/Footer/Footer'; | ||
| import Header from '@/components/Header/Header'; | ||
| import PrivateRoute from '@/components/PrivateRoute/PrivateRoute'; | ||
| import { AuthProvider} from '@/contexts/Authcontext'; | ||
|
|
||
| import Home from '../pages/Home'; | ||
| import Login from '../pages/Login'; | ||
| import MyAccount from '../pages/MyAccount'; | ||
| import Theme from '../pages/Theme'; |
There was a problem hiding this comment.
alias를 사용하거나 사용하지 않거나, 둘 중에 한 가지 방식으로 작성해주세요!
| const MyAccount: React.FC<MyAccountProps>= () => { | ||
| const navigate = useNavigate(); | ||
| const { user: username, logout } = useAuth(); | ||
|
|
||
| const handleLogout = () => { | ||
| logout(); | ||
| navigate('/'); | ||
| }; | ||
| return ( | ||
| <Container> | ||
| <StyledText>{username}님 안녕하세요!</StyledText> | ||
| <StyledButton onClick={handleLogout}>로그아웃</StyledButton> | ||
| </Container> | ||
| ); | ||
| } |
There was a problem hiding this comment.
앞선 리뷰를 참고해주세요!
지금의 경우에는 handleClickLogoutButton 처럼 네이밍을 해야 자연스러울 것 같아요!
| interface LoginProps { | ||
| onLogin: (username: string) => void; | ||
| } | ||
|
|
||
| const Login: React.FC<LoginProps> = ({}) => { |
| import GoodsCatygory from '@/components/GoodsCategory/GoodsCategory'; | ||
| import RankingHeader from '@/components/Ranking/RankingHeader'; | ||
| import SelectFriend from '@/components/SelectFriend/SelectFriend'; | ||
|
|
||
| import { Button } from '../components/common/Button/index'; | ||
| import Filter from '../components/Ranking/Filter/Filter'; |
There was a problem hiding this comment.
마찬가지로, alias를 사용하거나 사용하지 않거나, 둘 중에 하나만 하면 좋을 것 같아요!
| useEffect(() => { | ||
| const storedUser = sessionStorage.getItem('authToken'); | ||
| if (storedUser) { | ||
| setUser(storedUser); | ||
| } | ||
| }, []); | ||
|
|
||
| const login = (username: string) => { | ||
| sessionStorage.setItem('authToken', username); | ||
| setUser(username); | ||
| }; | ||
|
|
||
| const logout = () => { | ||
| sessionStorage.removeItem('authToken'); | ||
| setUser(null); | ||
| }; |
There was a problem hiding this comment.
authToken 이라는 문자열은 상수로 만들어서 사용해주면 어떨까요?!
| children: React.ReactNode; | ||
| } | ||
|
|
||
| const FilterButton: React.FC<FilterButtonProps> = ({ active, onClick, buttonText, children }) => { |
There was a problem hiding this comment.
components 에 있는 것들은 다양한 형태를 표현할 수 있어야 한다고 생각해요!
button 태그의 다른 props가 필요할 때에는 이 상황에서 어떻게 props type을 정의할 수 있을까요?
No description provided.