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

Conversation

eun-ko
Copy link

@eun-ko eun-ko commented Apr 17, 2020

https://react-vote-11th.eun-ko.now.sh/

매주 미션마다 난이도가 훅훅 올라가는거같아요
git사용법도 언제쯤 적응되려는지 계속 까먹네요ㅜㅜ
저번주에 useState에 이어 useEffect hook 사용법까지 익힐 수 있어서 좋았습니다.
voteForm구성하는게 어려워서 가장 많은 시간을 투자했던것같습니다!
매번 저의 부족함을 깨닫게됩니다 ..ㅎㅎ

Copy link
Member

@greatSumini greatSumini left a comment

Choose a reason for hiding this comment

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

정말정말정말정말정말 잘해주셨네요!
하루가 다르게 발전하는 모습이 보기 좋습니다 👍
선택 미션도 정말 잘해주셨어요!!
몇가지만 고쳐주시면 될 것 같아요 ㅎ
고생 많았습니다 오프라인 스터디에서 봐요~ 😄

Comment on lines +1 to +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"
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

굿입니다 👍

}

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 좋습니다 ㅎㅎ


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.

안녕


export default function LoginForm() {
return <Wrapper>안녕 나는 로그인 폼!</Wrapper>;
const [logForm, setLogForm] = useState({ email: '', password: '' });
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);

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

Comment on lines 51 to 54
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칸으로 조절하면 더 보기 좋을 것 같기도합니다 😄 오프라인 스터디에서 봐드릴게요!

Comment on lines 66 to 69
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);

Comment on lines +70 to +106
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`
Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 11 to 17
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] : ''
})
};

</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
같은 이름으로요 😄


function VoteForm() {
const [candi, setCandi] = useState([]);
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();

Copy link
Member

@greatSumini greatSumini left a comment

Choose a reason for hiding this comment

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

2차 리뷰입니다! 화이팅입니다 😅

const newUrl =
process.env.API_HOST + '/candidates/' + person._id + '/vote/';
const voteCandidates = async (candidate) => {
const newUrl = `${process.env.API_HOST}/candidates/${candidate._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가 명시되지 않은 경우, 안 넘겨주셔도 됩니다!

Comment on lines 14 to 15
const getCandidates = async () => {
await 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
const getCandidates = async () => {
await axios
const getCandidates = axios

이렇게 바꿔줘도 의미가 같습니다! axios가 비동기 함수이기 때문이죠

const newUrl =
process.env.API_HOST + '/candidates/' + person._id + '/vote/';
const voteCandidates = async (candidate) => {
const newUrl = `${process.env.API_HOST}/candidates/${candidate._id}/vote/`;
await axios
.put(newUrl, voteCount)
.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);

삭제해주시면 됩니다 ㅎㅎ

Comment on lines 49 to 54
.map((person, index) => (
<Row key={person._id}>
<Rank>{index + 1}위:</Rank>
<CandiName>
{person.name}
<br />[{person.voteCount}표]
Copy link
Member

Choose a reason for hiding this comment

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

candidate-card.js 이름으로 새 파일을 만들어서, CandidateCard라는 새로운 컴포넌트 작성한 뒤에
이 부분에서 아래와 같이 쓸 수 있도록 리팩토링 해보는건 어떨까요?

Suggested change
.map((person, index) => (
<Row key={person._id}>
<Rank>{index + 1}위:</Rank>
<CandiName>
{person.name}
<br />[{person.voteCount}]
.map((candidate, index) => (
<CandidateCard {...candidate} />
))

</Wrapper>
)}

{isloggedIn && <VoteForm />}
Copy link
Member

Choose a reason for hiding this comment

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

VoteForm이 LoginForm 내부에 있네요 😅
분리해주세요!

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.

3 participants