Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
23 changes: 23 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# See https://help.github.com/articles/ignoring-files/ for more about ignoring files.

# dependencies
/node_modules
/.pnp
.pnp.js

# testing
/coverage

# production
/build

# misc
.DS_Store
.env.local
.env.development.local
.env.test.local
.env.production.local

npm-debug.log*
yarn-debug.log*
yarn-error.log*
34 changes: 17 additions & 17 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
# github_star
목표: Git Star 목표 github user(또는 org) 를 검색하여 해당 user의 총 Repo 수와 총 star 수 그리고 repo별 스타들을 보여주는 page를 제작한다.
## Gitstar Ranking
---
React 환경

## 제작 세부 방법
1. Text Input을 통해 입력된 값을 사용하여 Github User 또는 Organization의 이름을 검색합니다.
### 실행방법
---
npm install
yarn start

2. 검색된 데이터를 사용하여 각 결과값의 상세값을 구하기 위한 API를 호출 합니다.
### 구현방법
---
상태 끌어올리기, hook, axios, ref, css

3. 검색 결과는 List로 표현되며 다음과 같은 UI로 구성됩니다.

![ui](./img/ui.png)

## 기타

1. 크로스 브라우징은 고려하지 않습니다.
2. JS/ TS뿐 아니라 Style 및 모든 라이브러리는 개인이 선호하는 어떤 것을 사용해도 됩니다.
3. 만약 위 이미지의 기획상 허점이 보인다면, 개인이 스스로 개선 및 변경하여 구현하여도 무방합니다.
4. 만약 구현이 힘든 부분이 있거나 더 나은 부분이 있다면 자유롭게 변경 가능합니다.(검색버튼 대신 입력 시 바로 자동검색 되게 한다던지)
5. 위 내용의 API는 github api를 이용하여 데이터를 가져옵니다.
https://developer.github.com/v3/ (어뷰징을 막기위해 하루에 너무 자주(한번에 수십 수백번) 요청하면 하루 API 제한이 걸릴 수 있습니다.)
### 구현 하고 나서
---
- 상태끌어올리기를 너무 많이 사용해서 context api로 리팩토링 해보고 싶다.
- css에 대한 지식이 너무 부족해 디자인이 만족스럽지 못하다.
- TS로 다시 해보고싶다.
- WebPack도 사용해보고 싶다....
- 지각 제출 죄송합니다 ㅠㅠㅠㅠㅠㅠ
36 changes: 36 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
"name": "github_star",
"version": "0.1.0",
"private": true,
"dependencies": {
"@testing-library/jest-dom": "^4.2.4",
"@testing-library/react": "^9.3.2",
"@testing-library/user-event": "^7.1.2",
"axios": "0.19.2",
Comment on lines +6 to +9

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

"node-sass": "4.13.1",
"react": "^16.13.1",
"react-dom": "^16.13.1",
"react-scripts": "3.4.1"
},
"scripts": {
"start": "react-scripts start",
"build": "react-scripts build",
"test": "react-scripts test",
"eject": "react-scripts eject"
},
"eslintConfig": {
"extends": "react-app"
},
"browserslist": {
"production": [
">0.2%",
"not dead",
"not op_mini all"
],
"development": [
"last 1 chrome version",
"last 1 firefox version",
"last 1 safari version"
]
}
}
Binary file added public/favicon.ico
Binary file not shown.
43 changes: 43 additions & 0 deletions public/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8" />
<link rel="icon" href="%PUBLIC_URL%/favicon.ico" />
<meta name="viewport" content="width=device-width, initial-scale=1" />
<meta name="theme-color" content="#000000" />
<meta
name="description"
content="Web site created using create-react-app"
/>
<link rel="apple-touch-icon" href="%PUBLIC_URL%/logo192.png" />
<!--
manifest.json provides metadata used when your web app is installed on a
user's mobile device or desktop. See https://developers.google.com/web/fundamentals/web-app-manifest/
-->
<link rel="manifest" href="%PUBLIC_URL%/manifest.json" />
<!--
Notice the use of %PUBLIC_URL% in the tags above.
It will be replaced with the URL of the `public` folder during the build.
Only files inside the `public` folder can be referenced from the HTML.
Unlike "/favicon.ico" or "favicon.ico", "%PUBLIC_URL%/favicon.ico" will
work correctly both with client-side routing and a non-root public URL.
Learn how to configure a non-root public URL by running `npm run build`.
-->
<title>React App</title>
</head>
<body>
<noscript>You need to enable JavaScript to run this app.</noscript>
<div id="root"></div>
<!--
This HTML file is a template.
If you open it directly in the browser, you will see an empty page.
You can add webfonts, meta tags, or analytics to this file.
The build step will place the bundled scripts into the <body> tag.
To begin the development, run `npm start` or `yarn start`.
To create a production bundle, use `npm run build` or `yarn build`.
-->
</body>
</html>
Binary file added public/logo192.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added public/logo512.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
25 changes: 25 additions & 0 deletions public/manifest.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"short_name": "React App",
"name": "Create React App Sample",
"icons": [
{
"src": "favicon.ico",
"sizes": "64x64 32x32 24x24 16x16",
"type": "image/x-icon"
},
{
"src": "logo192.png",
"type": "image/png",
"sizes": "192x192"
},
{
"src": "logo512.png",
"type": "image/png",
"sizes": "512x512"
}
],
"start_url": ".",
"display": "standalone",
"theme_color": "#000000",
"background_color": "#ffffff"
}
3 changes: 3 additions & 0 deletions public/robots.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# https://www.robotstxt.org/robotstxt.html
User-agent: *
Disallow:
7 changes: 7 additions & 0 deletions src/components/atoms/button/Button.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.button {
background-color: lightgray;
height: 3rem;
width: 5rem;
font-size: 1rem;
color: black;
}
10 changes: 10 additions & 0 deletions src/components/atoms/button/Button.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

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

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

대신

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

로 가져올 수 있어요! ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

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

import './Button.css'

export default function Button(props) {
return (
<button className="button" onClick={props.onClick}>
{props.name}
</button>
)
}

Choose a reason for hiding this comment

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

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

23 changes: 23 additions & 0 deletions src/components/atoms/item/Item.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
.item {
padding: 1rem;
display: flex;
transition: all 0.15s;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Suggested change
transition: all 0.15s;
transition: background 0.15s;

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

user-select: none;
justify-content: space-between;
}

.item:hover {
background: lightgray;
}

.item + .item {
border-top: 1px solid lightgray;
}

.star {
font-size: 2rem;
}

.name {
font-size: 2rem;
}
15 changes: 15 additions & 0 deletions src/components/atoms/item/Item.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import React from 'react'
import './Item.css'

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

좋은의견 감사합니다 ㅎㅎ

<a className="name">

Choose a reason for hiding this comment

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

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

{props.name}
</a>
<a className="star">
★ {props.stars}
</a>
</div>
)
}
6 changes: 6 additions & 0 deletions src/components/atoms/textInput/InputText.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
.input {
height: 2.5rem;
width: 50rem;
margin-right: 2rem;
font-size: 1.5rem;
}
8 changes: 8 additions & 0 deletions src/components/atoms/textInput/InputText.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import React from 'react';
import './InputText.css';

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

Choose a reason for hiding this comment

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

정말 nit 하긴 한데 다음과 같이 나눠 보는 것은 어떨까요??
가독성에 더 도움이 될 듯 해서요!

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

);
}
Comment on lines +4 to +8

Choose a reason for hiding this comment

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

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

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} />
);
}

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

아하! 감사합니다 ㅎㅎㅎ

4 changes: 4 additions & 0 deletions src/components/molecules/itemList/ItemList.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.itemList {
border: 1px solid lightgray;
margin: 1rem;
}
23 changes: 23 additions & 0 deletions src/components/molecules/itemList/ItemList.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import React from 'react'
import Item from '../../atoms/item/Item'
import './ItemList.css'

export default function ItemList(props) {

const repos = (props.repoList).map(
({id, name, starCount}) => (
<Item
key={id}
id={id}

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

@mango906 mango906 Apr 17, 2020

Choose a reason for hiding this comment

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

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

name={name}
stars={starCount}
/>
)
)

return (
<div className="itemList">
{repos}
</div>
)
}
3 changes: 3 additions & 0 deletions src/components/molecules/searchBar/SearchBar.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.SearchBar {

}
Comment on lines +1 to +3

Choose a reason for hiding this comment

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

요것은 놓치신 부분일까요? 아니면 의도하신 부분일까요?

22 changes: 22 additions & 0 deletions src/components/molecules/searchBar/SearchBar.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import React, {useRef} from 'react'
import Button from '../../atoms/button/Button'
import InputText from '../../atoms/textInput/InputText'
import {getRepoList} from '../../../service/getRepoList'

export default function SearchBar(props) {

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)}/>
Copy link

@YukJiSoo YukJiSoo Apr 16, 2020

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

</div>
Comment on lines +8 to +20

Choose a reason for hiding this comment

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

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>

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

)
Comment on lines +10 to +21

Choose a reason for hiding this comment

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

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 을 참조할 수 있으니 이렇게 바꿔도 괜찮을것 같습니다!

}
11 changes: 11 additions & 0 deletions src/components/molecules/userInfo/UserInfo.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
.userInfo {
margin-left: 2rem;
}

.nameinfo {
font-size: 3rem;
}

.repo {
font-size: 2rem;
}
16 changes: 16 additions & 0 deletions src/components/molecules/userInfo/UserInfo.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import React from 'react'
import './UserInfo.css'

export default function UserInfo(props) {
return (
Comment on lines +4 to +5

Choose a reason for hiding this comment

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

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

Suggested change
export default function UserInfo(props) {
return (
export default function UserInfo(props) {
const { userName, repoCount, starCount } = props;
return (

<div className="userInfo">
<a className="nameinfo">
{props.userName}
</a>
<br/>
<a className="repo">
{props.repoCount} Repositories | {props.starCount} stars
</a>
Comment on lines +7 to +13

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

좋은의견 감사합니다!

</div>
)
}
14 changes: 14 additions & 0 deletions src/components/organisms/header/Header.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
.Header {
background-color: lightgray;
margin: 1rem;
padding: 1rem;

}

.Header h1 {
font-size: 3rem;
}

.Header p {
font-size: 1.5rem;
}
13 changes: 13 additions & 0 deletions src/components/organisms/header/Header.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import React from 'react'
import SearchBar from '../../molecules/searchBar/SearchBar';
import './Header.css'

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

Choose a reason for hiding this comment

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

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

<h1>Gitstar Ranking</h1>
<p>Unofficial GitHub star ranking for users, organizations and repositories.</p>
<SearchBar userName={props.userName} setUserName={props.setUserName} setRepoList={props.setRepoList}/>
</div>
)
}
18 changes: 18 additions & 0 deletions src/components/organisms/searchResult/SearchResult.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import React from 'react'
import ItemList from '../../molecules/itemList/ItemList'
import UserInfo from '../../molecules/userInfo/UserInfo'

export default function SearchResult(props) {

const starCount = (ItemList) => {
return ItemList.reduce((acc, {starCount}) => acc += starCount, 0);
}
Comment on lines +7 to +9

Choose a reason for hiding this comment

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

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

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

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

<br/>
<ItemList repoList={props.repoList}/>
</div>
)
}
Loading