This repository was archived by the owner on Sep 19, 2021. It is now read-only.
[1주차] 고은 미션 제출합니다.#4
Open
eun-ko wants to merge 8 commits into
Open
Conversation
Author
ybh1760
reviewed
Apr 4, 2020
ybh1760
left a comment
ybh1760
left a comment
There was a problem hiding this comment.
안녕하세요. 이번 과제 리뷰를 맡게 된 양병훈입니다.
이번 기회에 처음으로 프론트개발을 시작하셨다고 들었는데, 너무 잘하셨습니다!
혹시나 리뷰 설명이 이해가 안 되시거나, 잘 모르겠는 부분에 대해서 리뷰 답글 남겨주세요!
정말 수고하셨습니다.😄
Comment on lines
+1
to
+4
| import React, { useState } from "react"; | ||
| import ProfileCard, { Prop, Row } from "../src/components/profile-card"; | ||
| import styled from "styled-components"; | ||
| import { render } from "react-dom"; |
There was a problem hiding this comment.
import문 작성 시에 외부 라이브러리는 위쪽에, 직접 작성한 컴포넌트는 한 줄아래에 작성하면
가독성이 좋아요😀
Suggested change
| import React, { useState } from "react"; | |
| import ProfileCard, { Prop, Row } from "../src/components/profile-card"; | |
| import styled from "styled-components"; | |
| import { render } from "react-dom"; | |
| import React, { useState } from "react"; | |
| import { render } from "react-dom"; | |
| import styled from "styled-components"; | |
| import ProfileCard, { Prop, Row } from "../src/components/profile-card"; |
| <ProfileCard /> | ||
| </Wrapper> | ||
| ); | ||
| let List = profiles.map((prop) => ( |
There was a problem hiding this comment.
객체나 배열은 재할당하는 경우가 생각보다 흔하지 않기 때문에,
const를 사용하면 의도치 않은 재할당을 방지해 주기 때문에 보다 안전하게 작성할 수 있습니다.
Suggested change
| let List = profiles.map((prop) => ( | |
| const List = profiles.map((prop) => ( |
Comment on lines
+62
to
+73
| <Prop | ||
| id={prop.id} | ||
| name={prop.name} | ||
| age={prop.age} | ||
| role={prop.role} | ||
| univ={prop.univ} | ||
| major={prop.major} | ||
| phoneNum={prop.phoneNum} | ||
| email={prop.email} | ||
| githubLink={prop.githubLink} | ||
| imageUrl={prop.imageUrl} | ||
| /> |
There was a problem hiding this comment.
몇 가지 개선할 수 있는 부분 알려드릴게요.
- react에 jsx spread attributes라는 패턴이 있습니다. 간단하게 말씀드리면 아래와 같이 spread operator를 사용하여 props을 전달하는 패턴입니다. 자세한 설명은 아래 링크 참고해주세요!
- React에서 배열을 element list로 만들 때, 데이터 변화에 유연하게 대응하기 위해서 (3개의 데이터 중 첫번째가 삭제됐을 때, 첫번째만 삭제하고 두세번째 UI는 리렌더하지 않기 위해) key라는 property를 사용해요! 자세한 개념은 공식문서 읽어보시는걸 추천합니다.
-
Suggested change
<Prop id={prop.id} name={prop.name} age={prop.age} role={prop.role} univ={prop.univ} major={prop.major} phoneNum={prop.phoneNum} email={prop.email} githubLink={prop.githubLink} imageUrl={prop.imageUrl} /> <Prop key={prop.id} {...prop} />
- 컴포넌트 네이밍 관련해서 의견입니다.
Prop이라는 이름을 보고 어떤 것을 보여주는 컴포넌트인지 바로 인지하기 어려운 거 같아요ㅜㅜ
여기서는 PropfileCard로 네이밍하는 것이 좋은 것 같습니다!
Comment on lines
+76
to
+80
| //return <div>{List}</div>;//4개한번에 다나옴. | ||
| //const [id_1, id1] = useState(List[0]); | ||
| //const [id_2, id2] = useState(List[1]); | ||
| //const [id_3, id3] = useState(List[2]); | ||
| //const [id_4, id4] = useState(List[3]); |
Comment on lines
+85
to
+93
| <Row> | ||
| <Wrapper>{List[0]}</Wrapper> | ||
|
|
||
| <Wrapper>{List[1]}</Wrapper> | ||
| </Row> | ||
| <Row> | ||
| <Wrapper>{List[2]}</Wrapper> | ||
| <Wrapper>{List[3]}</Wrapper> | ||
| </Row> |
There was a problem hiding this comment.
이 부분은 아래와 같이 수정할 수 있을 거 같아요
- Wrapper는 없애고 List에서 리턴하는 Prop에서 styling해주면 좋을 것 같네요.
- CardSection에서 사용되는 css 속성 관련 링크는 아래에 있습니다. 보시면 도움 되실 거에요!
Suggested change
| <Row> | |
| <Wrapper>{List[0]}</Wrapper> | |
| <Wrapper>{List[1]}</Wrapper> | |
| </Row> | |
| <Row> | |
| <Wrapper>{List[2]}</Wrapper> | |
| <Wrapper>{List[3]}</Wrapper> | |
| </Row> | |
| <CardSection> | |
| {List} | |
| </CardSection> |
const CardSection = styled.div`
display:flex;
flex-flow: row wrap;
justify-content: space-between;
`
Comment on lines
+8
to
+18
| id, | ||
| name, | ||
| age, | ||
| role, | ||
| univ, | ||
| major, | ||
| phoneNum, | ||
| email, | ||
| githubLink, | ||
| imageUrl | ||
| }) { |
| imageUrl | ||
| }) { | ||
| return ( | ||
| <div> |
There was a problem hiding this comment.
div대신에 여기서 Wrapper를 통해서 카드 styling하시면 좋을 것 같아요!
Comment on lines
+29
to
+31
| <div> | ||
| {univ}대학교 {major}과 | ||
| </div> |
There was a problem hiding this comment.
text를 감쌀때는 div태그 대신 p태그를 사용하는 게 좋아요.
div 태그는 웹페이지의 레이아웃을 구성할 때 자주 사용되고, p태그는 글이나 단락을 구성할 때 사용됩니다.
div태그와 p태그의 차이에 관한 자세한 설명은 아래 링크를 확인해주세요.
| </Row> | ||
|
|
||
| return <Wrapper>안녕 나는 프로필 카드</Wrapper>; | ||
| <CEOS> 신촌 연합 IT 창업 동아리 CEOS</CEOS> |
There was a problem hiding this comment.
Component의 이름은 무조건 PascalCase(첫글자가 대문자인 camelCase)로 작성해주셔야합니다!
Comment on lines
+42
to
+44
| <Img> | ||
| <img src={imageUrl} width="100%" /> | ||
| </Img> |
There was a problem hiding this comment.
styled-components를 통해서 img에 스타일을 줄 수 있어요!
Suggested change
| <Img> | |
| <img src={imageUrl} width="100%" /> | |
| </Img> | |
| <Img src={imageUrl} /> |
const Img = styled.img`
width: 40%;
height:40%;
`
Author
|
리뷰 반영했습니다! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
저도 커밋을 컴포넌트 단위로 추가하는걸 깜빡하고 있었어요ㅠㅠ다음부턴 꼭 참고하겠습니다! react를 처음 써봐서 새로 공부하면서 짜본 코드라 서툴겠지만 그래도 잘 봐주세요,,ㅎㅎ
https://react-profile-11th.eun-ko.now.sh/