Skip to content
This repository has been archived by the owner on Sep 19, 2021. It is now read-only.

[3주차] 최승원 미션 제출합니다. #6

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

seungwon2
Copy link

@seungwon2 seungwon2 commented Apr 17, 2020

개인적으로 시간을 너무 촉박하게 잡고 과제를 진행해서 아쉬운 부분이 많이 남습니다. 그리고 이상한 부분에서 계속 삽질을 해서,, 흑흑 시간이 조금 많이 모자랐던 것 같습니다. 서버 통신을 axios로 하는게 되게 간단하고 효율적인 것 같아 좋았습니다! memo는 어느 곳에 써야할지 아직은 감이 잘 안잡혀서 이부분도 더 공부하고 싶습니다.

https://react-vote-11th-sigma.now.sh/

Copy link
Contributor

@gywlsp gywlsp left a comment

Choose a reason for hiding this comment

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

안녕하세요! 코드 리뷰어 박효진입니다.
역시 승원님 잘 하시네용 👍 수고하셨습니당ㅎ
리뷰 확인해주시고 수정하면서 커밋한 후 push 부탁드려요!

pages/index.js Outdated

export default function Home() {
const [isLoggedIn, setLoginStatus] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [isLoggedIn, setLoginStatus] = useState(false);
const [isLoggedIn, setIsLoggedIn] = useState(false);

useState()로 state를 선언할 때, 보통 그 state의 set 함수를 set 뒤에 state 이름을 붙여 네이밍 해줍니다. 그 state에 값을 set하는 함수임을 명시적으로 나타내기 위해서요!

pages/index.js Outdated
리액트 투-표
<LoginForm />
<Title>리액트 투-표</Title>
{loginCheck()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{loginCheck()}
{!isLoggedIn && (<LoginForm/>)}
{isLoggedIn && (<VoteForm {...{setLoginStatus}}/>)}
  1. 컴포넌트들은 return() 안에서 확인할 수 있게 하면 좋아요. 굳이 컴포넌트를 리턴하는 함수를 만들지 말고 위와 같이 리팩토링하면 더 좋을 것 같아요 !
  2. console.log()는 commit할 때 다 지워주시는 게 좋아요!
  3. isLoggedIn<MemorizedLoginForm/>에 넘길 필요는 없을 것 같아요.
  4. props 이름과 넘길 값을 담고 있는 변수의 이름이 같다면 spread operator를 통해 props를 넘기면 좋아요!
  5. LoginForm, VoteForm에 모두 React.memo를 적용했지만 그렇다고 컴포넌트들을 import해서 쓸 때 Memoized를 붙여 import한 후 쓸 필요는 없어요. 참고

console.log(isLoggedIn);
})
.catch(function (error) {
initForm();
Copy link
Contributor

Choose a reason for hiding this comment

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

멋쪄용 👍
함수 이름을 다음과 같이 변경하면 역할을 더 잘 파악할 수 있을 것 같아용
resetForm()

Comment on lines 43 to 65
<Wrapper>
<LoginLabel>로그인</LoginLabel>
<InfoInputArea>
<EmailInputArea>
<EmailLabel>EMAIL</EmailLabel>
<EmailInput
name="email"
value={form.email}
onChange={handleFormChange}
/>
</EmailInputArea>
<PasswordInputArea>
<PasswordLabel>PASSWORD</PasswordLabel>
<PasswordInput
name="password"
value={form.password}
onChange={handleFormChange}
type="password"
/>
</PasswordInputArea>
</InfoInputArea>
<Submit onClick={handleSubmit}>로그인</Submit>
</Wrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Wrapper>
<LoginLabel>로그인</LoginLabel>
<InfoInputArea>
<EmailInputArea>
<EmailLabel>EMAIL</EmailLabel>
<EmailInput
name="email"
value={form.email}
onChange={handleFormChange}
/>
</EmailInputArea>
<PasswordInputArea>
<PasswordLabel>PASSWORD</PasswordLabel>
<PasswordInput
name="password"
value={form.password}
onChange={handleFormChange}
type="password"
/>
</PasswordInputArea>
</InfoInputArea>
<Submit onClick={handleSubmit}>로그인</Submit>
</Wrapper>
<Wrapper>
<Title>로그인</Title>
<InputWrapper>
<Row>
<Label>EMAIL</Label>
<Input
name="email"
type="text"
value={form.email}
onChange={handleFormChange}
/>
</Row>
<Row>
<Label>PASSWORD</Label>
<Input
name="password"
type="password"
value={form.password}
onChange={handleFormChange}
/>
</Row>
</InputWrapper>
<SubmitButton onClick={handleSubmit}>로그인</SubmitButton>
</Wrapper>
  1. 컴포넌트 네이밍을 위와 같이 해보는 건 어떨까요? 컴포넌트들의 역할을 더 잘 파악할 수 있게욥
  2. <EmailInputArea/>, <PasswordInputArea/>, <EmailLabel/>, <PasswordLabel/>, <EmailInput/>, <PasswordInput/>은 style이 같기 때문에 굳이 컴포넌트를 따로 정의할 필요가 없어 보여요.
    Label, Input같은 경우 Label 안과 Input name을 통해 어떤 것에 대한 label과 input을 파악할 수 있어 컴포넌트 네이밍에 굳이 또 명시할 필요는 없어 보입니당.
  3. Input에는 type을 써주시는 게 좋아요 ㅎ
  4. 핸들러 네이밍 좋네요 !

display: flex;
flex-direction: column;
`;
const LoginLabel = styled.p`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const LoginLabel = styled.p`
const LoginLabel = styled.label`

input의 label이기 때문에 p 태그 대신 label 태그를 써도 좋겠네용

Comment on lines 37 to 60
let i = 1;
return (
<Wrapper>
<Title>
<RedTitle>프론트엔드 인기쟁이</RedTitle>는 누구?
</Title>
<Desc>CEOS 프론트엔드 개발자 인기순위 및 투표창입니다.</Desc>
<VoteSection>
{candidates
.sort((a, b) => {
return b.voteCount - a.voteCount;
})
.map((candidate) => (
<List
key={candidate._id}
id={candidate._id}
i={i++}
{...candidate}
getCandidateList={getCandidateList}
/>
))}
</VoteSection>
</Wrapper>
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let i = 1;
return (
<Wrapper>
<Title>
<RedTitle>프론트엔드 인기쟁이</RedTitle> 누구?
</Title>
<Desc>CEOS 프론트엔드 개발자 인기순위 및 투표창입니다.</Desc>
<VoteSection>
{candidates
.sort((a, b) => {
return b.voteCount - a.voteCount;
})
.map((candidate) => (
<List
key={candidate._id}
id={candidate._id}
i={i++}
{...candidate}
getCandidateList={getCandidateList}
/>
))}
</VoteSection>
</Wrapper>
);
return (
<Wrapper>
<Title>
<Red>프론트엔드 인기쟁이</Red> 누구?
</Title>
<SubTitle>CEOS 프론트엔드 개발자 인기순위 및 투표창입니다.</SubTitle>
<VoteSection>
{candidates
.sort((a, b) => {
return b.voteCount - a.voteCount;
})
.map((candidate, index) => {
const {_id : id} = candidate;
return(
<CandidateCard
key={id}
{...candidate}
{...{getCandidateList}}
/>
)})}
</VoteSection>
</Wrapper>
);
  1. 컴포넌트 이름들을 조금 더 명시적으로 바꿔봤어용
  2. candidate 객체 안에 _idid라는 이름으로 destructuring 해서 쓰면 더 좋을 것 같아용 참고
  3. indexmap()의 parameter에서 받아서 넘겨주면 됩니다.
  4. idcandidate 안에 있어 spread operator로 넘겨주면 id가 넘어가서 따로 또 넘겨줄 필요는 없어요!
  5. props 이름과 넘길 값을 담은 변수의 이름이 같다면 spread operator를 이용해 넘겨주면 좋아용
  6. <List/> 컴포넌트 네이밍이 아쉬워요. 여러 명에 대한 정보가 담겨있지 않으니 List는 아닙니당. <CandidateCard/>와 같이 네이밍 하는 건 어떨까용

const VoteSection = styled.div`
width: 100%;
padding: 5rem 10rem;
border: 1px solid black initial;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
border: 1px solid black initial;
border: 1px solid black;

border-image: initial은 스타일 구현에 필요 없는 코드이기 때문에 initial을 지워주시면 됩니당.

Comment on lines 6 to 10
const Vote = () => {
axios
.put(process.env.API_HOST + "/candidates/" + id + "/vote/", {
params: {},
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const Vote = () => {
axios
.put(process.env.API_HOST + "/candidates/" + id + "/vote/", {
params: {},
})
const handleVote = () => {
axios
.put(process.env.API_HOST + `/candidates/${id}/vote/`)
  1. 이벤트 핸들러의 이름을 정의할 때는 앞에 보통 handle을 붙여줍니다.
  2. API document 보시면 해당 엔드포인트로 데이터를 보낼 때 body 객체를 보낼 필요가 없음을 확인할 수 있습니다.

Comment on lines 27 to 31
<VoteButton
onClick={() => {
Vote();
}}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<VoteButton
onClick={() => {
Vote();
}}
>
<VoteButton
onClick={handleVote}
>

onClick에는 실행시킬 함수를 넣어주면 됩니다. handleVote()는 따로 매개변수를 받지 않기 때문에 위와 같이 코드 작성해주시면 됩니다.

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

Successfully merging this pull request may close these issues.

2 participants