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_김현채 2주차 과제 #70

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

Conversation

HyeonChae1104
Copy link

@HyeonChae1104 HyeonChae1104 commented Jul 5, 2024

안녕하세요! 멘토님 피드백을 위해 시간 내주셔서 감사합니다!:) 지난주 피드백을 읽어보고 수정을 진행하였으나 제공해주신 파일이 셋팅이 더 잘되어있을 것 같아 제 과제가 아닌 제공 파일을 클론하여 2주차 진행을 하였습니다!

이번 주 과제가 제게는 많이 어렵게 느껴져서 시간이 정말 오래걸렸습니다.. 다음주 과제는 더 많은 시간을 쓰도록해 좋은 결과물을 제출하도록 노력하겠습니다!

밑은 질문사항입니다.

  1. 기능 구현 과정에서 불필요한 부분과 수정이 필요한 부분이 있을까요?
  2. 필터 기능을 hooks를 사용하여 구현하는 부분은(전체, 여성이, 남성이, 청소년이 / 받고 싶어한, 많이 선물한, 위시로 받은) 이후에 구현에 있어 테마 페이지처럼 다른 페이지로 연결되면 되는걸까요?

리액트의 사용이 처음이라 부족한 점이 많겠지만 잘 부탁드립니다!

@HyeonChae1104 HyeonChae1104 changed the title 전남대 FE_김현채 2주차 과제 제출합니다. 전남대 FE_김현채 2주차 과제 Jul 5, 2024
</div>
);
const PrivateRoute = () => {
const { authToken } = useAuth();
Copy link

Choose a reason for hiding this comment

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

루트 router 에서 토큰 유효성 검사를 진행한 후에 prop 으로 넘겨주면 어떨까요?

토큰의 존재 유무가 아닌 토큰 유효성 체크가 진행 되어야 하긴 합니다

import { useParams } from 'react-router-dom';

const CategoryPage: React.FC = () => {
const { themeKey } = useParams<{ themeKey: string }>();
Copy link

Choose a reason for hiding this comment

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

쿼리파람은 required 타입이 아닙니다. 옵셔널 및 방어코드가 필요해보이네요

</Banner>
<ProductList>
{products.map((product, index) => (
<ProductCard key={index}>
Copy link

Choose a reason for hiding this comment

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

index 를 키 값으로 주기보단 다른 필드를 활용해보면 어떨까요?

Comment on lines +7 to +9
<div className="footer">
<p className="footerText">카카오톡 선물하기</p>
</div>
Copy link

Choose a reason for hiding this comment

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

footer 태그 사용하면 더 시멘틱한 코드가 되겠네요 ㅎㅎ


const AuthContext = createContext<AuthContextType | undefined>(undefined);

export const AuthProvider: React.FC<{ children: ReactNode }> = ({ children }) => {
Copy link

Choose a reason for hiding this comment

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

API 가 없어서 한계가 있지만 위에 적은 코멘트 처럼 여기서 토큰 유효성 검증이 추가 되어야 합니다.

hasToken 과 isAuthenticated 는 다른 상태임을 짚고 넘어가면 좋을 것 같습니다.

const navigate = useNavigate();

const handleLogout = () => {
logout();
Copy link

Choose a reason for hiding this comment

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

private route 에서 토큰이 유실 된 후 다시 로그인 했을 때 경로를 유지시켜주면 더 좋겠네요 ㅎㅎ

@noah-zi
Copy link

noah-zi commented Jul 10, 2024

기능 구현 과정에서 불필요한 부분과 수정이 필요한 부분이 있을까요?

코드는 잘 작성해주셨고, 추가 코멘트는 리뷰 남겨두었습니다.

필터 기능을 hooks를 사용하여 구현하는 부분은(전체, 여성이, 남성이, 청소년이 / 받고 싶어한, 많이 선물한, 위시로 받은) 이후에 구현에 있어 테마 페이지처럼 다른 페이지로 연결되면 되는걸까요?

이후에 있을 기획의 방향에 따라 달라질 수 있긴하지만 지금은 맞습니다.

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.

2 participants