github_star_9 강영우 과제 제출합니다... #9

rdd9223 · 2020-04-15T22:43:56Z

정신 없이 지내다가 밤 새서 완성한 과제입니다... 스스로도 부족한 점이 많아보이지만 너그럽게 봐주시면 감사하겠습니다 ㅠㅠ Readme 참고하시면 실행할 수 있는 방법이 기술되어있고 부족한 부분은 따끔하게 지적해주세요!

src/components/organisms/searchResult/SearchResult.js

reduce 사용해서 객체들의 값 더하기! Co-Authored-By: curiousPark <[email protected]>

mango906 · 2020-04-16T10:50:50Z

저도 이번에 과제하면서 처음 적용해본건데 커밋할 때, 이렇게 규칙(?)을 정리해주면 더 좋을거 같아요!

늦은시간에 수고많으셨습니다 🤘🏻

https://jissi.tistory.com/10

lallaheeee

https://meetup.toast.com/posts/106

커밋에 조금 더 신경쓰시면 좋을 것 같아요 ㅎㅎ

lallaheeee · 2020-04-16T10:17:03Z

src/components/atoms/button/Button.js

@@ -0,0 +1,10 @@
+import React from 'react';


파일 이름을 index.js 로 하시면 나중에 import 할 때

import Buttom from '../../atoms/button/button'

대신

import Buttom from '../../atoms/button'

로 가져올 수 있어요! ㅎㅎ

희라님 코드 보면서도 궁금했었던 부분이었는데....! 꿀팁 감사합니다 ㅎㅎㅎ

lallaheeee · 2020-04-16T10:17:41Z

src/components/atoms/item/Item.css

+.item {
+  padding: 1rem;
+  display: flex;
+  transition: all 0.15s;


오 어떤 것을 위한 트랜지션인가요?! 호버에도 트랜지션이 되나요?! 궁금해요 !!

백그라운드 색을 바꿔보려고 했습니다 ㅎㅎㅎ

lallaheeee · 2020-04-16T10:21:03Z

src/service/getRepoList.js

+  }).catch(e => {
+    return [];
+  })


api 레이어에서 에러를 처리하면 컴포넌트에서 에러에 대한 적절한 처리가 어려울 것 같아요!

저도 희라님 의견에 동의합니다.

함수를 호출 하는 쪽에서 try catch 등을 활용해 에러를 핸들링 하거나,

ErrorHandler 등을 만들어 컨트롤해주면 좋을 것 같아요.

mango906 · 2020-04-16T10:35:50Z

src/components/atoms/item/Item.css

+.item {
+  padding: 1rem;
+  display: flex;
+  transition: all 0.15s;


background를 바꾸기 위해 transition 을 추가하신거 같은데 적용되는 대상이 하나라면 이렇게 작성하면 더 보기 좋을 거 같습니다!

Suggested change

transition: all 0.15s;

transition: background 0.15s;

이런 방법이...! 지금까지 부트스트랩만 쓰다가 css는 처음이라 많이 헷갈리네요ㅎㅎㅎ 감사합니다!

all은 의도하지 않은 css animation이 동작할 수 있어서 의도한 것만 명시하는 것이 좋아요!

mango906 · 2020-04-16T10:40:04Z

src/components/molecules/searchBar/SearchBar.js

+  const searchName = useRef();
+
+  const getRepoInfo = async(userName) => {
+    const result = await getRepoList(userName);
+    props.setRepoList(result);
+    props.setUserName(userName);
+  }
+
+  return (
+    <div className="SearchBar">
+      <InputText placeholder={"검색할 Github User 또는 Organization의 이름을 입력해주세요!"} searchName={searchName} />
+      <Button name="Search" onClick={() => getRepoInfo(searchName.current.value)}/>
+    </div>


ref를 사용하는거 말고도, 이런 방법으로도 가능할 것 같습니다 (손코딩 하느라 제데로 친지 헷갈리네요)

Suggested change

const searchName = useRef();

const getRepoInfo = async(userName) => {

const result = await getRepoList(userName);

props.setRepoList(result);

props.setUserName(userName);

}

return (

<div className="SearchBar">

<InputText placeholder={"검색할 Github User 또는 Organization의 이름을 입력해주세요!"} searchName={searchName} />

<Button name="Search" onClick={() => getRepoInfo(searchName.current.value)}/>

</div>

const [searchName, setSearchName] = useState("");

const getRepoInfo = async(userName) => {

const result = await getRepoList(userName);

props.setRepoList(result);

props.setUserName(userName);

}

const handleChange = useCallback((e)=> {

setUserName(e.target.value);

}, [setUserName]);

return (

<div className="SearchBar">

<InputText

placeholder={"검색할 Github User 또는 Organization의 이름을 입력해주세요!"}

onChange={handleChange} value={userName}}/>

<Button name="Search" onClick={() => getRepoInfo(userName)}/>

</div>

ref 말고 useCallback...! 기억하겠습니다 ㅎㅎㅎ 감사합니다!

<Button name="Search" onClick={() => getRepoInfo(userName)}/>
요 부분에서 핸들러가 인라인 함수로 들어가게 되면 렌더링 마다 새로운 함수로 인식하고 생성하게 되어서,
onClick={onSearchHandler} 와 같이 하는 것도 좋을 것 같습니다..!

mango906 · 2020-04-16T10:42:49Z

src/components/atoms/textInput/InputText.js

+export default function InputText(props) {
+  return (
+    <input className="input" type="text" placeholder={props.placeholder} ref={props.searchName}/>
+  );
+}


그럼 이부분도 이렇게 수정되어야할듯합니다

Suggested change

export default function InputText(props) {

return (

<input className="input" type="text" placeholder={props.placeholder} ref={props.searchName}/>

);

}

export default function InputText(props) {

const { placeholder, onChange, value } = props;

return (

<input className="input" type="text" placeholder={placeholder} onChange={onChange} value={value} />

);

}

아하....! ref를 직접 전달하지 않아도 되는건가요...!

ref를 전달한다기보단, onChange 이벤트 걸어서 값을 보낸다고 생각하시면 이해하시기 편하실듯합니다!

아하! 감사합니다 ㅎㅎㅎ

mango906 · 2020-04-16T10:44:49Z

src/components/molecules/userInfo/UserInfo.js

+      <a className="nameinfo">
+        {props.userName}
+      </a>
+      <br/>
+      <a className="repo">
+        {props.repoCount} Repositories | {props.starCount} stars
+      </a>


a 태그로 하신 이유가 있을까요???

글자 크기를 조정해주고 싶었는데 생각나는게 a태그밖에 없어서 우선 그걸로 했던 것 같아요 ㅠㅠ
a태그 말고 더 좋은 태그나 방법이 있을까요???

p태그나 span태그가 가장 적당하다고 생각됩니다

좋은의견 감사합니다!

mango906 · 2020-04-16T10:46:40Z

src/components/templates/GithubStar.js

+  const [userName, setUserName] = useState("");
+  const [repoList, setRepoList] = useState([]);
+
+  return (
+    <div>
+      <Header userName={userName} setUserName={setUserName} setRepoList={setRepoList}/>
+      {!repoList.length ? null:(<SearchResult repoList={repoList} userName={userName}/>)}


null대신 Fragment가 들어가면 더 좋겠다는 개인적인 생각(?)입니다!

&&연산자도 좋을 것 같아요!

{repoList.length && <SearchResult repoList={repoList} userName={userName}/>}

아....! 감사합니다 ㅎㅎ

저도 && 연산자를 더 선호해요!!

mango906 · 2020-04-16T10:47:47Z

src/service/getRepoList.js

+  return await axios.get(
+    `https://api.github.com/users/${userName}/repos`
+  ).then(({data}) => {
+    const result = data.map(({
+      id,
+      name,
+      stargazers_count
+    }) => ({
+      id,
+      name,
+      starCount: stargazers_count
+    }))
+    return result;


오 저는 데이터를 받아와서 그대로 리턴시켰는데 이렇게 쓸 값들만 뽑아올수 있네요 👍

YukJiSoo · 2020-04-16T14:31:25Z

src/components/molecules/itemList/ItemList.js

+    ({id, name, starCount}) => (
+      <Item
+        key={id}
+        id={id}


Itemcomponent 파일을 보니 id props가 사용되고 있지 않는 것 같아보여요.
불필요한 props는 넘겨주지 않는것이 좋다고 생각해요!!

map함수로 렌더링할때 key값이 필요한데 index로 넣으면 안좋다는 이야기를 어디서 들은것 같아서 id값을 넣은건데 잘모르겠네요...ㅎㅎ 좋은의견 감사합니다!🤩

Item 컴포넌트에 key말고도 id={id}를 보내주는데 지수님은 이 부분이 딱히 필요없다고 하시는거 같아요

YukJiSoo · 2020-04-16T14:38:13Z

src/components/molecules/searchBar/SearchBar.js

+  return (
+    <div className="SearchBar">
+      <InputText placeholder={"검색할 Github User 또는 Organization의 이름을 입력해주세요!"} searchName={searchName} />
+      <Button name="Search" onClick={() => getRepoInfo(searchName.current.value)}/>


Button component의 onClick props에 바로 함수를 명시해주게 되면 컴포넌트가 렌더링 될 때마다 새로운 함수가 생성되기 때문에
최적화에 문제가 있다고 알고 있어요!
Button에 넘겨줄 함수를 component 내에서 선언하고 넣어주도록 하는 것이 어떨까요??

컴포넌트 렌더링하고 구색만 맞추기만 할줄알아서 최적화는 생각도 못하고 있었네요....! 좋은 팁 감사합니다!🙆‍♂️

YukJiSoo · 2020-04-16T14:43:27Z

src/components/templates/GithubStar.js

+  const [userName, setUserName] = useState("");
+  const [repoList, setRepoList] = useState([]);
+
+  return (
+    <div>
+      <Header userName={userName} setUserName={setUserName} setRepoList={setRepoList}/>
+      {!repoList.length ? null:(<SearchResult repoList={repoList} userName={userName}/>)}


&&연산자도 좋을 것 같아요!

{repoList.length && <SearchResult repoList={repoList} userName={userName}/>}

joi0104 · 2020-04-16T14:50:56Z

src/serviceWorker.js

+      // Ensure service worker exists, and that we really are getting a JS file.
+      const contentType = response.headers.get('content-type');
+      if (
+        response.status === 404 ||


에러코드 상수화 시켜면 가독성이 더 좋을 것 같아요 ㅎㅎ

네 ㅎㅎ 좋은의견 감사합니다 serviceWorker는 다뤄보지 않았는데 다음엔 한번 적용해볼게요!

eastroots92

재미있게 잘 봤습니다!

eastroots92 · 2020-04-17T13:56:29Z

package.json

+    "@testing-library/jest-dom": "^4.2.4",
+    "@testing-library/react": "^9.3.2",
+    "@testing-library/user-event": "^7.1.2",
+    "axios": "0.19.2",


평소에 ^를 지우시는 편이신가요??
놓치신 것인지 아니면 지우는 기준이 있는지 궁금합니다.

음... 신경 안쓰고 있었는데 ^가 없네요.... ^의 유무에 차이가 있나요??

eastroots92 · 2020-04-17T13:57:43Z

src/components/atoms/button/Button.js

+      {props.name}
+    </button>
+  )
+}


전체적으로 eol이 놓쳐지고 있는 것 같아요 챙겨주시면 좋을 것 같습니다.

eastroots92 · 2020-04-17T13:58:27Z

src/components/atoms/item/Item.css

+.item {
+  padding: 1rem;
+  display: flex;
+  transition: all 0.15s;


all은 의도하지 않은 css animation이 동작할 수 있어서 의도한 것만 명시하는 것이 좋아요!

eastroots92 · 2020-04-17T13:59:13Z

src/components/atoms/item/Item.js

+export default function Item(props) {
+  return (
+    <div className="item">
+      <a className="name">


여기에 a tag를 사용하신 이유가 있을까요?

eastroots92 · 2020-04-17T13:59:34Z

src/components/atoms/item/Item.js

+
+export default function Item(props) {
+  return (
+    <div className="item">


item인 경우 li 등을 활용해 주시면 좀 더 시멘틱한 페이지를 만들 수 있습니다.

좋은의견 감사합니다 ㅎㅎ

eastroots92 · 2020-04-17T14:04:32Z

src/components/molecules/userInfo/UserInfo.js

+export default function UserInfo(props) {
+  return (


요런식으로 한번 구조분해 해서 가져가도 좋을 것 같아요!

Suggested change

export default function UserInfo(props) {

return (

export default function UserInfo(props) {

const { userName, repoCount, starCount } = props;

return (

eastroots92 · 2020-04-17T14:05:03Z

src/components/organisms/header/Header.js

+
+export default function Header(props) {
+  return (
+    <div className="Header">


Header를 의도하신 것 이라면 Header와 같이 시멘틱 태그를 사용해주시면 더 좋을 것 같아요!

eastroots92 · 2020-04-17T14:05:41Z

src/components/organisms/searchResult/SearchResult.js

+  const starCount = (ItemList) => {
+   return ItemList.reduce((acc, {starCount}) =>  acc += starCount, 0);
+  }


스윽 이렇게도 가능할 것 같아요!

Suggested change

const starCount = (ItemList) => {

return ItemList.reduce((acc, {starCount}) => acc += starCount, 0);

}

const starCount = (ItemList) => ItemList.reduce((acc, {starCount}) => acc += starCount, 0)

오... 생략할 수 있죠 놓친부분 감사합니다 ㅎㅎ

eastroots92 · 2020-04-17T14:07:38Z

src/components/organisms/searchResult/SearchResult.js

+
+  return (
+    <div>
+      <UserInfo repoCount={(props.repoList).length} starCount={starCount(props.repoList)} userName={props.userName}/>


전체적으로 데이터를 한번 mapping(?), 가공해서 사용하면 어떨까요?

props를 넘겨주는 부분에서 계산하는 로직이 추가되다 보니 가독성이 조금 떨어지는 것 같아서요!

eastroots92 · 2020-04-17T14:08:07Z

src/components/templates/GithubStar.js

+  const [userName, setUserName] = useState("");
+  const [repoList, setRepoList] = useState([]);
+
+  return (
+    <div>
+      <Header userName={userName} setUserName={setUserName} setRepoList={setRepoList}/>
+      {!repoList.length ? null:(<SearchResult repoList={repoList} userName={userName}/>)}


저도 && 연산자를 더 선호해요!!

danivelop · 2020-04-17T15:04:01Z

src/components/molecules/searchBar/SearchBar.js

+  const getRepoInfo = async(userName) => {
+    const result = await getRepoList(userName);
+    props.setRepoList(result);
+    props.setUserName(userName);
+  }
+
+  return (
+    <div className="SearchBar">
+      <InputText placeholder={"검색할 Github User 또는 Organization의 이름을 입력해주세요!"} searchName={searchName} />
+      <Button name="Search" onClick={() => getRepoInfo(searchName.current.value)}/>
+    </div>
+  )


Suggested change

const getRepoInfo = async(userName) => {

const result = await getRepoList(userName);

props.setRepoList(result);

props.setUserName(userName);

}

return (

<div className="SearchBar">

<InputText placeholder={"검색할 Github User 또는 Organization의 이름을 입력해주세요!"} searchName={searchName} />

<Button name="Search" onClick={() => getRepoInfo(searchName.current.value)}/>

</div>

)

const getRepoInfo = useCallback(async () => {

const userName = searchName.current.value;

const result = await getRepoList(userName);

props.setRepoList(result);

props.setUserName(userName);

}, []);

return (

<div className="SearchBar">

<InputText placeholder={"검색할 Github User 또는 Organization의 이름을 입력해주세요!"} searchName={searchName} />

<Button name="Search" onClick={getRepoInfo}/>

</div>

)

@YukJiSoo 님 말씀대로 컴포넌트의 props로 동적으로 함수를 생성해주게 되면 리렌더링 시마다 새로운 함수가 할당되 이를 props로 넘겨받은 컴포넌트가 불필요하게 리렌더링 될 수 있습니다. 여기서는 getRepoInfo 함수 안에서 searchName 을 참조할 수 있으니 이렇게 바꿔도 괜찮을것 같습니다!

rdd9223 added 9 commits April 16, 2020 00:33

Initialize project using Create React App

409dc94

first commit

61d5c63

뼈대완성

1c21f1f

버튼 스타일 추가

a1de58a

버튼 스타일 수정

4988d17

아이템 스타일 수정

6167be8

추가수정

e0a5843

전체 스타일 수정

a0fb6f1

리드미 고치기

d74777f

NuclearPunch reviewed Apr 16, 2020

View reviewed changes

src/components/organisms/searchResult/SearchResult.js Outdated Show resolved Hide resolved

reduce 사용하기!

3f2db5f

reduce 사용해서 객체들의 값 더하기! Co-Authored-By: curiousPark <[email protected]>

lallaheeee reviewed Apr 16, 2020

View reviewed changes

mango906 reviewed Apr 16, 2020

View reviewed changes

YukJiSoo reviewed Apr 16, 2020

View reviewed changes

joi0104 reviewed Apr 16, 2020

View reviewed changes

eastroots92 reviewed Apr 17, 2020

View reviewed changes

danivelop reviewed Apr 17, 2020

View reviewed changes

github_star_9 강영우 과제 제출합니다... #9

Are you sure you want to change the base?

github_star_9 강영우 과제 제출합니다... #9

Conversation

rdd9223 commented Apr 15, 2020

mango906 commented Apr 16, 2020 • edited Loading

lallaheeee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eastroots92 Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mango906 Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

YukJiSoo Apr 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eastroots92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mango906 commented Apr 16, 2020 •

edited

Loading

eastroots92 Apr 17, 2020 •

edited

Loading

mango906 Apr 17, 2020 •

edited

Loading

YukJiSoo Apr 16, 2020 •

edited

Loading