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

[3주차] 고은 미션 제출합니다. #4

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,5 @@ yarn-error.log*
.env.development.local
.env.test.local
.env.production.local
.env*
.env*
.now
13 changes: 3 additions & 10 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,11 +1,4 @@
# react-vote-11th

## 실행 방법

```
npm install
npm run dev
```

- npm install : 필요한 모든 패키지를 설치합니다. 처음 1번만 실행하면 됩니다.
- npm run dev : react(next) 웹서버를 localhost:3000에서 실행합니다.
vote-form을 구현하는데 시간을 많이 썼던것같습니다
axios.put에서 url로 각 후보별 id에 해당하는 인자를 어떻게 넘겨줄지가 어려웠고, 투표 버튼을 클릭해서 투표 완료시 화면에 업데이트 하는점도 어려웠습니다 로그인 오류시 폼 초기화하는걸 getElementById로 하느라 input에 props 전달할때 name으로 안하고 id로 해서 [e.target.id]: e.target.value로 했는데 맞는지 모르겠어요..
useEffect에 대한 이해도 아직 부족하고 axios사용도 많이 미숙한것같습니다ㅠㅠ!
12 changes: 12 additions & 0 deletions now.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"version": 2,
"public": false,
"builds": [{ "src": "next.config.js", "use": "@now/next" }],
"build": {
"env": {
"NODE_ENV": "@react-vote-11th-node-env",
"PORT": "@react-vote-11th-port",
"API_HOST": "@react-vote-11th-api-host"
}
}
}
Comment on lines +1 to +12
Copy link
Member

Choose a reason for hiding this comment

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

굿입니다 👍

31 changes: 31 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"start": "node server"
},
"dependencies": {
"axios": "^0.19.2",
"compression": "^1.7.4",
"dotenv": "^8.2.0",
"express": "^4.17.1",
Expand Down
28 changes: 15 additions & 13 deletions pages/index.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
import React from "react";
import styled from "styled-components";
import React from 'react';
import styled from 'styled-components';

import LoginForm from "../src/components/login-form";
import LoginForm from '../src/components/login-form';

export default function Home() {
return (
<Wrapper>
리액트 투-표
<LoginForm />
</Wrapper>
);
return (
<Wrapper>
<Title>리액트 투-표</Title>
<LoginForm />
</Wrapper>
);
}

const Title = styled.h1`
Copy link
Member

Choose a reason for hiding this comment

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

semantic tag 좋습니다 ㅎㅎ

font-size: 40px;
`;
const Wrapper = styled.div`
min-height: 100vh;
padding: 10rem 40rem;
background-color: Azure;
min-height: 100vh;
padding: 10rem 40rem;
background-color: Azure;
`;
112 changes: 104 additions & 8 deletions src/components/login-form.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,110 @@
import React from "react";
import styled from "styled-components";
import React, { useState } from 'react';
import styled from 'styled-components';
import axios from 'axios';

import VoteForm from './vote-form';

export default function LoginForm() {
return <Wrapper>안녕 나는 로그인 폼!</Wrapper>;
const [logForm, setLogForm] = useState({ email: '', password: '' });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [logForm, setLogForm] = useState({ email: '', password: '' });
const [loginForm, setLoginForm] = useState({ email: '', password: '' });

loginForm이 더 적절한 네이밍 같아요!

Copy link
Member

Choose a reason for hiding this comment

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

안녕

const [isloged, setloged] = useState(false);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [isloged, setloged] = useState(false);
const [isloggedIn, setloggedIn] = useState(false);

위와 마찬가지로 네이밍 변경해주세요~


const erasePW = () => {
document.getElementById('password').value = '';
};

const eraseEmail = () => {
document.getElementById('email').value = '';
};
Copy link
Member

Choose a reason for hiding this comment

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

  1. 여기서 2중 화살표 함수를 사용할 수 있겠네요 ㅎ
    호출시엔 다음과 같이 사용하면 됩니다.
erase('email')();
erase('password')();
  1. dom에 직접 접근하기보단 react state를 사용해주세요!
Suggested change
const erasePW = () => {
document.getElementById('password').value = '';
};
const eraseEmail = () => {
document.getElementById('email').value = '';
};
const erase = (name) => () => {
setLoginForm({
...loginForm,
[name] : ''
})
};


const handleFormChange = (e) => {
setLogForm({ ...logForm, [e.target.id]: e.target.value });
};

const handleSubmit = (logForm) => {
const { email, password } = logForm;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const handleSubmit = (logForm) => {
const { email, password } = logForm;
const handleSubmit = () => {
const { email, password } = loginForm;

handleSubmit 함수내에서 loginForm에 바로 접근할 수 있기 때문에 parameter로 넘겨주지 않아도 될 것 같아요!

if (email === '' || password === '') {
alert('모든 항목을 입력해주세요!');
return false;
} else {
axios
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (email === '' || password === '') {
alert('모든 항목을 입력해주세요!');
return false;
} else {
axios
if (email === '' || password === '') {
alert('모든 항목을 입력해주세요!');
return false;
}
axios

위의 if문에 해당된 경우 return문이 실행되며 함수가 종료되기 때문에, 이후 실행되는 line들은 모두 해당 조건에 충족되지 않는다고 봐도 되겠죠?
예시 참고해서 리팩토링 해보세요 ㅎㅎ! 예시링크
더 참고할만한 글

.post(process.env.API_HOST + '/auth/signin/', logForm)
.then(function (response) {
Copy link
Member

Choose a reason for hiding this comment

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

화살표함수로 바꿔보세요~

console.log(response);
setloged(true);
alert('로그인 성공!');
})
.catch(function (error) {
if (error.response.status === 404) {
console.log('unauthorized, logging out ...');
alert('이메일이 존재하지 않습니다.');
eraseEmail();
erasePW();
} else if (error.response.status === 422) {
alert('비밀번호가 일치하지 않습니다!');
erasePW();
}
Copy link
Member

Choose a reason for hiding this comment

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

선택과제도 해주셨네요 ㅠㅠ 대단합니다

return Promise.reject(error.response);
});
}
};

return (
<div>
{!isloged && (
<Wrapper>
Copy link
Member

Choose a reason for hiding this comment

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

indentation space가 4칸으로 설정돼있는 것 같아요
2칸으로 조절하면 더 보기 좋을 것 같기도합니다 😄 오프라인 스터디에서 봐드릴게요!

<Title>로그인</Title>
<Row>
<Label>EMAIL</Label>
<Input type="text" id="email" onChange={handleFormChange}></Input>
Copy link
Member

Choose a reason for hiding this comment

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

id 사용 좋네요~! name으로도 바꿔보세요!

Suggested change
<Input type="text" id="email" onChange={handleFormChange}></Input>
<Input type="text" name="email" onChange={handleFormChange}></Input>

</Row>
<Row>
<Label>PASSWORD</Label>
<Input
type="password"
onChange={handleFormChange}
id="password"
></Input>
</Row>
<Button onClick={() => handleSubmit(logForm)}>로그인</Button>
</Wrapper>
)}

{isloged && <VoteForm />}
</div>
);
}
const Title = styled.h1`
font-size: 3rem;
margin-bottom: 4rem;
margin-top: 25px;
`;
const Row = styled.div`
display: flex;
flex-direction: row;
margin-bottom: 2rem;
`;
const Button = styled.button`
display: block;
margin-left: auto;
font-size: 1.8rem;
padding: 0.5rem 1rem;
border-style: none;
border-radius: 1rem;
`;

const Label = styled.label`
font-size: 20px;
margin-right: auto;
`;
const Input = styled.input`
width: 75%;
padding: 0.5rem 1rem;
border: 1px solid grey;
`;
const Wrapper = styled.div`
width: 100%;
min-height: 30rem;
background-color: white;
font-size: 18px;
padding: 3rem 4rem;
width: 100%;
min-height: 30rem;
background-color: white;
font-size: 18px;
padding: 3rem 4rem;
`;
125 changes: 116 additions & 9 deletions src/components/vote-form.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,121 @@
import React from "react";
import styled from "styled-components";
import React, { useState, useEffect } from 'react';
import axios from 'axios';

export default function VoteForm() {
return <Wrapper>안녕 나는 투표 폼!</Wrapper>;
import styled from 'styled-components';

function VoteForm() {
const [candi, setCandi] = useState([]);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [candi, setCandi] = useState([]);
const [candidates, setCandidates] = useState([]);
  1. initial value로 빈 배열 넘겨주신 것 정말정말정말정말정말x100 좋습니다!! 🥰
  2. 변수,함수명에 약어를 사용하지 말아주세요! 알아보기 쉬운 코드가 좋은 코드입니다 :)
  3. 배열을 담고있는 변수이름은 복수형으로 써주세요! (ex : profiles, animals, candidates)

const [voteCount, setVoteCount] = useState();
Copy link
Member

@greatSumini greatSumini Apr 18, 2020

Choose a reason for hiding this comment

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

이 state가 사용되지 않고 있어요!, vote시 paramter로 넘겨줄 필요가 없기 때문에 지워주시면 될 것 같아요!

Suggested change
const [voteCount, setVoteCount] = useState();


useEffect(() => {
getCandidates();
}, []);

const getCandidates = async () => {
await axios
.get(process.env.API_HOST + '/candidates/', candi)
.then(({ data }) => {
setCandi(data);
})
.catch(function (error) {
console.log(error);
});
Copy link
Member

Choose a reason for hiding this comment

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

catch도 화살표함수로 써주세요 😄

};

const voteCandidates = async (person) => {
Copy link
Member

Choose a reason for hiding this comment

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

person보다 candidate라는 parameter name이 좋을 것 같아요

Suggested change
const voteCandidates = async (person) => {
const voteCandidates = async (candidate) => {

const newUrl =
process.env.API_HOST + '/candidates/' + person._id + '/vote/';
Copy link
Member

Choose a reason for hiding this comment

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

아래처럼 바꿀 수 있습니다!

Suggested change
const newUrl =
process.env.API_HOST + '/candidates/' + person._id + '/vote/';
const newUrl = `${process.env.API_HOST}/candidates/${person._id}/vote/`;

await axios
.put(newUrl, voteCount)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.put(newUrl, voteCount)
.put(newUrl)

request body schema가 명시되지 않은 경우, 안 넘겨주셔도 됩니다!

.then(({ data }) => {
setVoteCount(data);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
setVoteCount(data);

삭제해주시면 됩니다 ㅎㅎ

alert(person.name + '님에게 투표 완료!');
getCandidates();
})
.catch(function (error) {
console.log(error);
alert('투표 실패!');
});
};
let i = 1;
return (
<Wrapper>
<Title>
<RedTitle>프론트엔드 인기쟁이</RedTitle>는 누구?
</Title>
<SubTitle>CEOS 프론트엔드 개발자 인기 순위 및 투표 창입니다.</SubTitle>
<VoteArea>
{candi
.sort((person1, person2) => {
return person2.voteCount - person1.voteCount;
})
Copy link
Member

Choose a reason for hiding this comment

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

아래처럼 바꿀 수 있습니다!

Suggested change
.sort((person1, person2) => {
return person2.voteCount - person1.voteCount;
})
.sort((person1, person2) => person2.voteCount - person1.voteCount )

.map((person) => (
<Row key={JSON.stringify(person)}>
<Rank>{i++}위:</Rank>
Copy link
Member

Choose a reason for hiding this comment

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

  1. JSON.stringify 사용해주신 것 정말 좋네요! 그런데 이 상황에선 id가 넘어오기 때문에 그 값을 key로 이용하면 될 것 같습니다 ㅎ
  2. i 라는 별도의 변수를 사용하는 것보다 map 함수의 index를 사용하는게 더 좋을 것 같아요!
Suggested change
.map((person) => (
<Row key={JSON.stringify(person)}>
<Rank>{i++}위:</Rank>
.map((person, index) => (
<Row key={person._id}>
<Rank>{index + 1}위:</Rank>

<CandiName>
{person.name}
<br />[{person.voteCount}표]
</CandiName>
<Button onClick={() => voteCandidates(person)}>투표</Button>
</Row>
))}
Copy link
Member

Choose a reason for hiding this comment

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

candidate 한명 한명에 대한 component를 따로 분리해서 map 해보는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

엇 이게 map안에서 적용되고 있는거 아닌가요..??

Copy link
Member

Choose a reason for hiding this comment

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

앗 넵 다른 파일로 분리해보시면 어떨까해서 드린 말씀이었어요!
CandidateCard
같은 이름으로요 😄

</VoteArea>
</Wrapper>
);
}
export default React.memo(
VoteForm,
(prev, next) => prev.voteCount === next.voteCount
);
Copy link
Member

Choose a reason for hiding this comment

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

VoteForm component에는 prop이 없죠! React.memo는 prop을 비교해서 리렌더를 결정하기 때문에, prop이 없는 component는 아래처럼 비교함수를 안 써주시면 됩니다.

Suggested change
export default React.memo(
VoteForm,
(prev, next) => prev.voteCount === next.voteCount
);
export default React.memo(VoteForm);

const Row = styled.div`
display: flex;
flex-direction: row;
align-items: center;
`;
const Button = styled.button`
background-color: navy;
color: white;
font-size: 1.5rem;
padding: 0.5rem 1rem;
border-style: none;
border-radius: 1rem;
`;
const Rank = styled.strong`
font-size: 1.5rem;
margin: 0rem 4rem 1rem 0rem;
`;
const CandiName = styled.p`
font-size: 1.5rem;
display: block;
margin: 0rem auto 1rem 0rem;
`;
const VoteArea = styled.div`
width: 100%;
padding: 5rem 10rem;
border: 1px solid black;
display: flex;
flex-direction: column;
justify-content: space-between;
`;
const Title = styled.span`
font-size: 3rem;
color: black;
display: inline-block;
font-weight: bold;
`;
const RedTitle = styled.span`
Comment on lines +64 to +100
Copy link
Member

Choose a reason for hiding this comment

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

용도 별로 다른 태그 사용하신거 너무 잘하셨어요~~ 💯

font-size: 3rem;
color: crimson;
`;
const SubTitle = styled.h1`
font-size: 2.5rem;
color: grey;
`;

const Wrapper = styled.div`
width: 100%;
min-height: 30rem;
background-color: white;
font-size: 18px;
padding: 3rem 4rem;
width: 100%;
min-height: 30rem;
background-color: white;
font-size: 18px;
padding: 3rem 4rem 10rem;
`;