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

[김한수] 3,4주차 과제 제출 #20

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

Conversation

gillyongs
Copy link

image

-local storage로 작성중인 글 내용 저장 구현

-recoil로 전역 변수 관리

-카테고리 검색 기능 추가

-useForm과 yup으로 회원가입, 로그인 구현

-styled components 적용

저번주에 제출못한 3주차 과제와 이번 4주차 과제입니다

회원가입 파트는 너무 어려워서 가은님이 작성한 코드를 이해하는 방향으로 공부했습니다 ㅎㅎ;

대신이라하긴 뭐하지만 주어진 api 대신 직접 spring api 구현에서 연결하였습니다!

https://khs20010327.tistory.com/142

https://khs20010327.tistory.com/143

Copy link
Collaborator

@chchaeun chchaeun left a comment

Choose a reason for hiding this comment

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

안녕하세요 한수님! 3, 4주차 미션까지 진행하시느라 고생 많으셨어요. 로그인에 대해 공부하는 계기가 되었으면 좋겠네요. 리뷰 남겨두었으니 참고해주세요.
추가적으로 말씀 드리고 싶은 부분은 지금 디렉터리 구조가 조금 불명확한 것 같아서요. 디렉터리 구조에 정답은 없지만 page를 다루는 부분과 util 함수들을 다루는 부분, 공통적으로 사용되는 component를 다루는 부분, 페이지 내부 component를 다루는 부분 이런 식으로 좀 더 명확한 기준을 가지면 좋을 것 같단 생각이 드네요.
또, 로그인을 통해 API를 연결했으니 다른 부분(제품 조회, 글쓰기)도 API를 연결해보시면 좋을 것 같아요. 모르는 부분이 있으시면 언제든 물어보세요. :)

import styled from "styled-components";
import BackButton from './BackButton'

function Header2({title}){
Copy link
Collaborator

Choose a reason for hiding this comment

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

Header2, Header3는 이 컴포넌트가 각각 무슨 역할을 위해 분리가 되었는지 명확하지 않은 네이밍인 것 같아요. 또 두 컴포넌트 간의 차이가 CSS 빼고 없는 것 같아서 CSS를 props 조건에 따라 바꿔주고, 두 컴포넌트를 하나로 합치는 게 낫지 않을까 싶습니다

<Header2Style>
<BackButton />
<h1> {title} </h1>
<h2> 완료 </h2>
Copy link
Collaborator

Choose a reason for hiding this comment

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

h2는 버튼 기능을 하지 않습니다! 역할에 맞는 태그를 사용하면 좋을 것 같아요. 그리고 이벤트 리스너가 등록되어있지 않은데 아직 이부분 구현이 안된 걸까요?

function HeartButton({heart}){
const [url, setUrl] = useState("/ui/heart_button.png");
return(
<HeartStyle onClick={()=>{setUrl("/ui/red_heart.png")}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 하트를 다시 눌렀을 때 취소가 되게 하려면 떻게 하는 게 좋을까요?


export default HeartButton;

const HeartStyle = styled.div
Copy link
Collaborator

Choose a reason for hiding this comment

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

컴포넌트 이름에 style은 빼시고 역할 기반으로 작성해주세요. 또한 div는 논시맨틱 태그입니다! 시맨틱 태그 사용해주세요

});
}

const [loginState, setLoginState] = useState({
Copy link
Collaborator

Choose a reason for hiding this comment

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

state 관련 코드는 상단에 위치해야 관리가 쉽습니다!

{(errors.pw)?<p>{errors.pw.message}</p>:""}
</label>
{
loginError?
Copy link
Collaborator

Choose a reason for hiding this comment

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

조건이 false일 때 출력할 내용이 없는 경우 삼항연산자가 아닌 &&를 사용하면 좀 더 깔끔하게 작성할 수 있어요.

이메일로 로그인해주세요.
</h2>
<label>이메일(ID)
<input {...register("email")} name="email" type="text" onInput={onInput}/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

onChange가 아닌 onInput을 사용한 이유가 있을까요?

setIsLoggedIn((prevState) => ({
...prevState,
state: true,
accessToken: response.data.accessToken,
Copy link
Collaborator

Choose a reason for hiding this comment

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

구조분해할당을 사용하면 좀 더 깔끔할 것 같아요

accessToken: response.data.accessToken,
refreshToken: response.data.refreshToken,
nickName: response.data.nickName,
userId: response.data.userId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

유저 아이디와 닉네임은 스토리지에 저장하지 않아도 됩니다. 일반적으로 api를 통해 불러오는데요. xss와 csrf 공격에 대해 찾아보시면 어떨까요?

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