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

과제 제출 #8

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

과제 제출 #8

wants to merge 32 commits into from

Conversation

lallaheeee
Copy link
Member

🌈 간단 설명

  • 입력된 값을 사용하여 Github User 또는 Organization의 이름을 검색
  • 검색된 데이터를 사용하여 각 결과값의 상세값을 구하기 위한 API를 호출
  • 검색 결과는 List로 표현되며 다음과 같은 UI로 구성

🌎 환경

JS, React, Sass

🏡 배포 URL

https://lallaheeee-gitstar.netlify.com/

react에 필요한 환경 설정과
lint, code fomatting 과 관련된 설정을 완료
../../../ 경로가 길어지는 것을 막고자
webpack에서 alias 설정
그에 따라 eslint에서 에러 발생해서 eslint 설정 또한 추가
웹 폰트 적용 및
필요없는 헤더 제거 및 그에 따른 main 스타일 변경
검색 버튼에 사용할 버튼 컴포넌트 작성
검색창 input에 대한 UI 작성 완료
searchBar 컴포넌트 UI 작성 완료
star svg 및 카운트 수를 나타내는 컴포넌트 UI 작성
검색한 유저나 organization에 따라
총 레포수, 스타 수를 노출하는 컴포넌트 UI 작성
검색 결과에 따라 레포의 이름과 스타 수를 노출할 컴포넌트 ui 작성
ref 쓰려는데 오류나서왜지 보니까
input 하나 컴포넌트로 분리해서였다ㅎㅎ..
그래서 삭제 후 molecules에 그냥 input을 넣었다 ㅎㅎ
에러나서 li로 key를 옮겨줌
이름, 레포 수, 스타 수, 레포 리스트가 노출되는
컴포넌트 작성
alias 추가 및 src 경로 추가
비동기 통신에 사용할 모듈 설치
repo를 조회하는 api 요청 작성
검색어 입력에 따라 레포 수, 스타 수, 레포 목록을 노출하는
GitStarRanking 컴포넌트 작성
정확한 레포 오너 대소문자가 나타나도록 변경
operator-linebreak 나 object-curly-newline 등
eslint --fix와 prettier onSave 가 달라서 설치
1. import/no-cycle
- index.js 삭제

기타 코드 스타일 수정
API 요청 로딩시 스피너를 노출하려고 npm 다운
API 요청 로딩시 스피너 노출 추가
더불어 클릭시 input 창의 value가 비워지도록 수정
@mango906
Copy link

mango906 commented Apr 14, 2020

레포에 대한 아이템들이 쭉 나열되면서 border가 겹치는 현상이 있는데 ul.search_result밑의 li 컴포넌트들에게 & + li { margin-top: -1px } 스타일을 입히면 더 좋을거 같습니다! (사진으론 잘 안보이네요)

스크린샷 2020-04-14 오후 2 03 44

[예시]

스크린샷 2020-04-14 오후 2 09 28

요청을 처리하기 전이나 응답을 받기전 어떤 처리를 해주려 했으나
그런 일이 없었음
이에 따라 병합
화면에 렌더링하지 않는 방법이 더 깔끔해서 변경
useState 때보다 코드가 길어졌지만
액션을 명시해주고 싶어서 변경
style.scss import가 되어있지 않아 수정
기타 스타일 수정
@lallaheeee
Copy link
Member Author

lallaheeee commented Apr 14, 2020

@mango906
margin-top: -1px 옵션을 주면 해결되는데 4개마다 또 굵게 보여서 아예 떼어버렸어요 ㅎㅎㅎ
이런 경우엔 어떻게 하면 좋을까요?

@mango906
Copy link

@lallaheeee
SearchResultList 컴포넌트의 li에 주신거 맞나요?? 저는 잘 나와서요!

스크린샷 2020-04-14 오후 8 45 56

repoCount,
starCount,
resultItems,
}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

대부분의 코드에서 destructuring을 활용하신 것 같은데, 혹시 이유가 있을까용?
개인적으로 destructuring을 사용하면 편리한 부분도 있는데, 가져와야하는 변수가 많은 경우에는 쬐에끔 가독성이 저하되는 경우도 있는 것 같아서요ㅠㅠ

Copy link
Member Author

Choose a reason for hiding this comment

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

ㅎㅎ 타입스크립트가 아니다보니 각 컴포넌트에 필요한 props를 명시해주고 싶었습니다 ㅎㅎ
코드스타일인거 같아요 ㅎㅎ

Choose a reason for hiding this comment

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

@lallaheeee PropTypes를 정의해주는거도 하나의 방법일거 같습니다!

https://reactjs.org/docs/typechecking-with-proptypes.html

Copy link
Member Author

Choose a reason for hiding this comment

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

@mango906 ㅋㅋㅋㅋ 경빈님 한 pr당 스무번은 클릭하셨죠!!

Choose a reason for hiding this comment

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

@lallaheeee 저 새로운거 올라올떄마다 계속 보고있어요 ㅋㅋㅋㅋㅋㅋ


![ui](./img/ui.png)
### ✨ [Demo](https://lallaheeee-gitstar.netlify.com/)
Copy link
Collaborator

Choose a reason for hiding this comment

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

demo까지! 대단하심다...👍😍

});
})
.catch(err => {
if (err.response.status === 404) {

Choose a reason for hiding this comment

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

오..에러핸들링까지 짱짱이시네요
404에러가 아닐 경우의 처리도 하시면 더 짱짱짱이실 것 같아요!

Choose a reason for hiding this comment

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

에러코드들을 상수화해서 처리하는 방법도 있을 것 같아요!

Copy link
Member 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.

2020-04-17_22-43-33

NotFound 페이지 귀엽네요 ㅋㅋㅋㅋ

Comment on lines +40 to +42
options: {
presets: ['@babel/preset-env', '@babel/preset-react'],
},

Choose a reason for hiding this comment

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

확실치는 않지만 root에 있는 babelrc 파일에 preset을 명시해주었기 때문에 이 부분은 필요 없지 않을까요??


***
_This README was generated with ❤️ by [readme-md-generator](https://github.com/kefranabg/readme-md-generator)_
Copy link

Choose a reason for hiding this comment

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

설명이 자세해서 보기가 좋아요!

const API = axios;

export function getRepos(keyword) {
return API.get(`/users/${keyword}/repos`).then(({ data }) => {
Copy link

Choose a reason for hiding this comment

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

Organization에 대한 검색이 빠져있는듯 합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

/users로 해도 Organization 의 레포 결과들을 가져오더라고요ㅎㅎ

Choose a reason for hiding this comment

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

엇??! 생각지도 못한 허점이닼ㅋㅋㅋ처음 알았어요

Copy link

@YukJiSoo YukJiSoo 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 +21 to +29
{
test: /\.(js|jsx)$/,
include: path.resolve(__dirname, '../src'),
enforce: 'pre',
loader: 'eslint-loader',
options: {
emitWarning: true,
},
},

Choose a reason for hiding this comment

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

dev build 과정에 eslint-loader를 추가해주신 이유가 따로 있으신가요?? 궁금해요!

Copy link
Member Author

Choose a reason for hiding this comment

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

린트 에러 놓친 부분을 한번 더 알려주라고 추가했어요 ㅎㅎ

Comment on lines +52 to +61
"husky": {
"hooks": {
"pre-commit": "lint-staged"
}
},
"lint-staged": {
"*.{js,jsx,json,css,scss}": [
"git add"
]
}

Choose a reason for hiding this comment

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

husky랑 lint-staged도 적용하셨네여 👍!!
이 설정도 별도 파일로 분리할 수 있는 걸로 알고 있는데 어떤가여??

https://github.com/okonet/lint-staged#lintstagedrc-example
https://github.com/typicode/husky#upgrading-from-014

Comment on lines +50 to +56
<PageBody
status={{ loading, isNotFound }}
name={keyword}
resultItems={repos}
repoCount={repos.length}
starCount={repos.reduce((acc, cur) => acc + cur.starCount, 0)}
/>

Choose a reason for hiding this comment

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

PageBody 컴포넌트의 props로 repoCountstarCount는 불필요해 보여요!

PageBody안에서 data를 받아온 이후에 SearchResult component를 렌더링 해주도록 작성하신것 같은데 ,
SearchResult component 내에서 component 내부에서 resultItems property를 이용해서 충분히 도출해낼 수 있는 것 같아요!

Copy link
Member Author

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 +29
export const Action = {
SEARCH_LOADING: 'SEARCH_LOADING',
SEARCH_SUCCESS: 'SEARCH_SUCCESS',
SEARCH_NOT_FOUND: 'SEARCH_NOT_FOUND',
SEARCH_FINISH: 'SEARCH_FINISH',
};

export const initialState = {
keyword: null,
repos: [],
loading: false,
isNotFound: false,
};

const reducers = {
[Action.SEARCH_LOADING]: state => ({ ...state, loading: true }),
[Action.SEARCH_SUCCESS]: (state, { keyword, repos }) => ({
...state,
keyword,
repos,
}),
[Action.SEARCH_NOT_FOUND]: state => ({ ...state, isNotFound: true }),
[Action.SEARCH_FINISH]: state => ({ ...state, loading: false }),
};

export default (state, action) => {
const reducer = reducers[action.type];
return reducer ? reducer(state, action.payload) : state;
};

Choose a reason for hiding this comment

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

reducer 이런 방식으로 사용하는 것 좋은 것 같아요~ 👏👏

Comment on lines +21 to +23
const handleOnClick = () => {
searchInput.current.value = '';
};

Choose a reason for hiding this comment

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

input을 클릭할 때 입력되어 있던 값이 clear되게 하려고 하신거 같은데 onFocus event는 어떠신가요오??

Copy link
Member Author

Choose a reason for hiding this comment

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

좋아요 ㅎㅎ

},
],
},
};
Copy link

Choose a reason for hiding this comment

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

webpack까지 사용하시다니,, 너무 좋은것같아요!👍

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 유림님 좋아요 : ) ❤️

({
id,
name,
description,
Copy link

Choose a reason for hiding this comment

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

혹시 repos에 데이터의 description도 저장하신 이유가 있나요??🙋

Copy link
Member Author

@lallaheeee lallaheeee Apr 16, 2020

Choose a reason for hiding this comment

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

오래돼서 저장한 기억이 안나는데 그당시에 description도 노출하고 싶었나봐요 ㅎㅎ
리팩토링하면서 description도 노출해야겠어요 ㅎㅎ 놓친 부분 잡아주셔서 감사합니다!

Comment on lines +28 to +30
'@atoms': path.resolve(__dirname, '../src/components/atoms'),
'@molecules': path.resolve(__dirname, '../src/components/molecules'),
'@organisms': path.resolve(__dirname, '../src/components/organisms'),

Choose a reason for hiding this comment

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

아토믹 디자인기법은 실제론 처음 보는거같아요 나중에 기회가 된다면 관련 주제로 세미나 또는 스터디해도 좋을거 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

🥴🐣🥳

Comment on lines +14 to +22
return (
<div className="search_result">
<SearchResultDescription
name={name}
repoCount={repoCount}
starCount={starCount}
/>
<SearchResultList resultItems={resultItems} />
</div>
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
Member Author

Choose a reason for hiding this comment

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

요기 요 ㅎㅎ

{resultItems.map(({ id, url, ...props }) => (
<li key={id}>
<a href={url} target="_blank" rel="noopener noreferrer">
<SearchResultListItem {...props} id={id} />

Choose a reason for hiding this comment

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

SearchResultListItem 컴포넌트에 속성으로 전달되는 id 가 사용되지 않는것 같아서 id는 삭제해도 될것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

오 그러네요 ㅎㅎ 감사합니다 ㅎㅎ

Comment on lines +10 to +23
const handleOnSearch = () => {
onSearch(searchInput.current.value);
searchInput.current.focus();
};

const handleKeyUp = ({ key }) => {
if (key !== ENTER) return;

handleOnSearch();
};

const handleOnClick = () => {
searchInput.current.value = '';
};

Choose a reason for hiding this comment

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

함수형 컴포넌트에서는 리렌더링 될때마다 handleOnSearch , handleKeyUp , handleOnClick 에 새로운 함수가 할당되기 때문에 useCallback로 감싸서 메모이제이션 해주면 좀더 메모리나 렌더링 성능이 좋아질 수 있을것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

https://rinae.dev/posts/review-when-to-usememo-and-usecallback

저는 이글을 읽고 useCallback과 useMemo 사용을 지양하는 편인데요
@danivelop 님의 생각은 어떠신가유?!

Comment on lines +27 to +33
<input
placeholder="github user 또는 organization을 검색해보세요."
ref={searchInput}
onKeyUp={handleKeyUp}
onClick={handleOnClick}
/>
<Button name="search" onClick={handleOnSearch} />

Choose a reason for hiding this comment

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

form 태그도 한번 사용해보시는 것도 괜찮을것 같습니다

Copy link

@eastroots92 eastroots92 left a comment

Choose a reason for hiding this comment

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

와 디테일이 살아있네요

config/webpack.dev.js Show resolved Hide resolved
const API = axios;

export function getRepos(keyword) {
return API.get(`/users/${keyword}/repos`).then(({ data }) => {

Choose a reason for hiding this comment

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

엇??! 생각지도 못한 허점이닼ㅋㅋㅋ처음 알았어요


const API = axios;

export function getRepos(keyword) {

Choose a reason for hiding this comment

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

함수 선언식과 화살표 함수가 혼재 되어 사용하고 있는 듯 한데 구분 짓는 기준이 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

동근님 매의 눈이시네요 ㅎㅎ
화살표 함수가 깔끔하게 느껴져서 개인적으로 선호하는 편인데요.
작성한 과제를 보니 혼재되어 있네요 ㅎㅎ
무의식적으로 snippet 을 사용하다보니 이렇게 된 것 같습니다 !
다시보니 스타일이 일관되지 않는다는 느낌이 드네요
다음부터는 이런 스타일도 챙겨야겠어요 🌝🥴

@@ -0,0 +1,32 @@
import axios from 'axios';

axios.defaults.baseURL = 'https://api.github.com';

Choose a reason for hiding this comment

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

평소에 axiosInstance를 선언해서 사용하는 편인데 이런식으로 axios 자체에도 baseURL을 직접 설정 할 수 있군요.

Copy link
Member Author

Choose a reason for hiding this comment

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

미세먼지 팁인데,, 당연히 알고 계실거 같지만 ㅎㅎ
interceptor를 이용해 요청이 되기전이나 되고난후에 처리도 할 수 있어요!! 👍🏻
https://yamoo9.github.io/axios/guide/interceptors.html

Choose a reason for hiding this comment

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

@lallaheeee 오 이걸로 그럼 async, await 대체 할수 있는건가요??

Choose a reason for hiding this comment

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

@mango906
음 async, await랑은 조금 다른 성격일듯 해요.

interceptors는 API에 request 하기 전 response 값을 return 하기 전에 잠시 자기가 원하는 event들을 실행하는 용도로 많이 활용됩니다!

예를들면
response interceptors 에는 의도하지 않은 Error 404, 500 등을 핸들링 해줄 수 있어요. (여기서 처리하면 에러핸들링을 한번만 해줘도 되겠죠?)

Comment on lines +28 to +29
keyword: owner,
repos,

Choose a reason for hiding this comment

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

eslint에서 airbnb 룰을 사용하셨던데 린트에서 아래와 같이 하라고 제안 안했었나요??

airbnb 룰을 사용하면 다음과 같이 하라 했던 기억이 있어서요.

Suggested change
keyword: owner,
repos,
repos,
keyword: owner,

Copy link
Member Author

Choose a reason for hiding this comment

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

음 저는 못봤는데 둘이 어떤 차이가 있을까요?! 무슨 룰인지 궁금하네요 !!
prettier 플러그인도 같이 사용중이라 prettier 플러그인을 지워봐도 뜨지 않아요 ㅜㅜ

Choose a reason for hiding this comment

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

정렬 순서와 관련된 룰로 알고 있는데요. Airbnb 룰에는

key와 value가 같아 shorthand로 사용 가능하면 shorthand로,
shorthand가 가능한 것과 불가능한 것이 혼재 되어있을 경우 shorthand 먼저 명시하는 룰이 있어요!

https://github.com/airbnb/javascript#objects--grouped-shorthand

@@ -0,0 +1,27 @@
.search_result_item {
& {

Choose a reason for hiding this comment

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

nit 한 궁금증인데

& { ... } 로 해도 의도한 대로 나올 듯 한데 이렇게 하는 이유가 있을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

제가 css selector에 대해 많이 아는 편이 아니라 이해를 잘 못했는데 & { ... } 로 해도가 어떻게 하는 걸까요?! 🤔

Choose a reason for hiding this comment

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

아 제가 글을 잘못 적었네요.

현재 & { style들 } 과 같이 &로 선택자를 지정해 주셨는데, 저 부분은 & 선택자를 안써도 의도한 대로 동작 할 듯 해서요!

<ul className="search_result">
{resultItems.map(({ id, url, ...props }) => (
<li key={id}>
<a href={url} target="_blank" rel="noopener noreferrer">

Choose a reason for hiding this comment

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

오 target에 rel까지 최고에요!! 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

이런 칭찬 완전 좋아요 🥳 감사합니다

});
})
.catch(err => {
if (err.response.status === 404) {

Choose a reason for hiding this comment

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

2020-04-17_22-43-33

NotFound 페이지 귀엽네요 ㅋㅋㅋㅋ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants