mash-up 과제 by danivelop · Pull Request #1 · mash-up-kr-web/github_star_9

danivelop · 2020-03-19T06:53:47Z

유저이름으로 깃허브 레포지토리 및 스타갯수 얻기

lallaheeee

오 리덕스는 사용해보지 않았는데 재밌어 보이네요 👍🏻 고생하셨습니다 😊

lallaheeee · 2020-04-15T11:30:20Z

@@ -0,0 +1 @@
+export { default } from './Loading';


Loading.tsx의 로직을 index.tsx에 작성하지 않고 이렇게 한번 더 나누신 이유가 있을까요?! 궁금해요 ㅎㅎ

@lallaheeee

아마
import 시에 @/components/Loading/Loading 이렇게 임포트 path가 정해지는 것을 피하고 싶으셔서 그랫 던 것이 아닐까?
하는 추측이 있습니다

Loading컴포넌트의 역할을 이름으로 알기쉽게 하기위해 index파일에는 작성하지 않았고 @eastroots92 님 말씀대로 임포트를 간단하게 하기위해서도 그랬습니다ㅎㅎ

lallaheeee · 2020-04-15T11:37:24Z

+  return (
+    <ul className={cx('repo-list')}>
+      {
+        repos.map((repo, index) => <RepoItem key={index} repo={repo} />)


response에 각 레포마다 id 도 있던데 key 를 id를 주면 어떨까요?! ㅎㅎ

Suggested change

repos.map((repo, index) => <RepoItem key={index} repo={repo} />)

repos.map(({id, ...repo }, index) => <RepoItem key={id} repo={repo} />)

https://ko.reactjs.org/docs/lists-and-keys.html#keys

오 희라님 의견 동의합니다.
추가로 index params 또한 생략 가능하겠네요!

Suggested change

repos.map((repo, index) => <RepoItem key={index} repo={repo} />)

repos.map((repo) => <RepoItem key={repo.id} repo={repo} />)

아 그렇네요. 지금 확인해보니 github api의 리턴값으로 id속성이 있네요. 감사합니다!

jieunjieun · 2020-04-15T12:13:50Z

+          </ul>
+        </article>
+        {
+          !!repos.length && (


!!를 붙이신 이유가 있나용? 굳이 boolean으로 바꿔주지 않아도 truthy, falsy value가 잘 들어가니까 의도한 대로 잘 동작 할 거 같아서요!

아마 제 경험에 비추어봤을 때 reps.length가 화면에 노출돼서 일것 같아요 ㅎㅎ

저는 불린 값을 써야 할때 !!를 붙여줘서 boolean값인지 명시하는걸 좋아하는데 다른 분들 의견은 어떠신가요?

저는 뭔가 글 처럼 스르륵 읽히는걸 좋아해서 boolean 으로 캐스팅 + 값 체크되는 유틸 함수를 한번 감싸서 사용했던 것 같아용. 생각해보니 좀 번거롭네요 ㅠㅠ
근데 !! 가 눈에 익으면 간편해서 좋을 것 같아요!

!!를 안붙이면 repos.length가 0일때 값이 그대로 렌더링이 되서 !!를 붙여주여 명시적으로 boolean 값으로 바꿔주었습니다

@Yuni-Q
저도 처음에는 의도적으로 붙였었는데,
예전에 어떤 아티클을 읽고나서 뗏던것같아요. (그 아티클이 꽤 시간이 지나서 찾아봐야할 듯 하네요)

TMI. airbnb나 google 스타일 가이드 룰에도 뗀다고 되어있어요!

YukJiSoo

redux를 사용해본적이 없어서 코드를 처음 봤는데 재밌어요! 고생하셨습니다ㅎㅎ

YukJiSoo · 2020-04-15T19:22:38Z

+  "dependencies": {
+    "@testing-library/jest-dom": "^4.2.4",
+    "@testing-library/react": "^9.5.0",
+    "@testing-library/user-event": "^7.2.1",
+    "@types/jest": "^24.9.1",
+    "@types/node": "^12.12.30",
+    "@types/react": "^16.9.23",
+    "@types/react-dom": "^16.9.5",
+    "axios": "^0.19.2",
+    "classnames": "^2.2.6",
+    "immer": "^6.0.2",
+    "node-sass": "^4.13.1",
+    "react": "^16.13.0",
+    "react-dom": "^16.13.0",
+    "react-redux": "^7.2.0",
+    "react-scripts": "3.4.0",
+    "redux": "^4.0.5",
+    "redux-saga": "^1.1.3",
+    "typescript": "^3.7.5"
+  },


typescript나 types~ 같이 실제 브라우저 환경에서 실행되는데 필요없는 라이브러리 들은 devDependencies에 있는게 좋을 것 같아요!

create-react-app 으로 리액트 프로젝트 설정하면 typescript 나 관련 @types 파일들이 dependencies로 들어가더라구요. 저도 왜그런지 궁금하긴 했는데 @YukJiSoo 님 말씀대로 devDependencies로 옮겨야 할 것 같아요!

YukJiSoo · 2020-04-15T19:33:57Z

+  return (
+    <Loading loading={loading}>
+      {
+        () => (
+          <MainComponent
+            repos={repos}
+            username={username}
+            onChangeInput={handleChangeInput}
+            onSearch={handleSearch}
+          />
+        )
+      }
+    </Loading>
+  );


바로 component를 넘겨도 정상적으로 작동할 것 같다고 생각하는데 위의 방법으로 작성해주신 이유가 있으신가요??

Suggested change

return (

<Loading loading={loading}>

{

() => (

<MainComponent

repos={repos}

username={username}

onChangeInput={handleChangeInput}

onSearch={handleSearch}

/>

)

}

</Loading>

);

return (

<Loading loading={loading}>

<MainComponent

repos={repos}

username={username}

onChangeInput={handleChangeInput}

onSearch={handleSearch}

/>

</Loading>

);

그리고, Loading component가 MainComponent를 감싸고 있는 형태로 작성해주신 이유가 있을까요??
loading state에 따라서 Loading component를 보여줄지 안 보여줄지 결정해도 될것 같다고 생각해서요!

저도 궁금합니다.

이 부분에 관해서 다른 방법으로 LoadingComponent 상속 받아서 하는거나
customhooks 만들어 봐도 좋을거 같아요 !
경험적인 측면에서

저도 처음에 @YukJiSoo님 말씀대로 loading 값에 따라 조건분기해서 렌더링을 해줬었는데 그냥 프로젝트 하다가 Loading컴포넌트는 렌더속성값으로 바꿔주고 싶어져서 저렇게 했습니다ㅎㅎ 사실 렌더링비용 측면에선 @YukJiSoo님이 말씀해주신 방법대로 하는게 더 좋을듯 합니다

mango906 · 2020-04-15T12:47:12Z

+.loading-img {
+  width: 100vw;
+  height: 100vh;
+  position: fixed;
+  top: 0;
+  left: 0;
+  display: flex;
+  justify-content: center;
+  align-items: center;
+  background-color: rgba(255, 255, 255, 0.9);
+  z-index: 99;
+
+  img {
+    width: 200px;
+    height: 200px;
+  }
+}


top값과 left값을 사용하실거면 이렇게 쓰는게 좀 더 보기 좋을거 같습니다!

Suggested change

.loading-img {

width: 100vw;

height: 100vh;

position: fixed;

top: 0;

left: 0;

display: flex;

justify-content: center;

align-items: center;

background-color: rgba(255, 255, 255, 0.9);

z-index: 99;

img {

width: 200px;

height: 200px;

}

}

.loading-img {

position: fixed;

top: 0;

left: 0;

right: 0;

bottom: 0;

display: flex;

justify-content: center;

align-items: center;

background-color: rgba(255, 255, 255, 0.9);

z-index: 99;

img {

width: 200px;

height: 200px;

}

}

감사합니다! 저런방법으로도 작성해 봐야겠어요

mango906 · 2020-04-15T12:51:52Z

+  const handleSearch = useCallback((e: React.FormEvent<HTMLFormElement>) => {
+    e.preventDefault();
+    dispatch(getRepo(username));
+  }, [username, dispatch]);


e.preventDefault() 를 추가하신 이유가 있으실까요??

form에 기본 action 이벤트가 있어서 기본이벤트 없애주기 위해 form태그의 이벤트에는 항상 써주게 되더라구요! 사실 안써도 크게 문제는 없을거 같습니다ㅎㅎ

eastroots92

전체적으로 eol과 code style 등이 일관되지 않은 듯 한데

eslint 등을 활용해보시면 좋을 것 같아요!

eastroots92 · 2020-04-16T02:32:42Z

@@ -0,0 +1,23 @@
+# See https://help.github.com/articles/ignoring-files/ for more about ignoring files.


정말 nit 하긴 한데

협업하는 상황에서 ide의 설정을 통일하려고 하는 것이 아니라면
ide의 설정 파일들도 제외시켜주면 좋을 것 같아요

저는 개인적으로 vscode와 webstorm 정도 제외하고 있습니다.

eastroots92 · 2020-04-16T02:38:26Z

@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html lang="en">


정말 nit 하긴 한데 한글 서비스이니 ko로 바꾸는 것은 어떤가요?

Suggested change

<html lang="en">

<html lang="ko">

오 미처 신경을 못썻던 부분입니다! 감사합니다

eastroots92 · 2020-04-16T02:39:20Z

+    <!--
+      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/
+    -->


요것도 nit한 부분인데

과제여서 그러셨을 수 도 있지만, 불필요한 부분은 지워주시면 좋을 것 같아요!

index.html파일은 신경을 못썼는데 저부분도 놓치면 안될것 같네요

eastroots92 · 2020-04-16T02:43:02Z

+  );
+};
+
+export default App;


요기 EOL 챙겨주시면 좋을 것 같아요!

요런 부분들은 놓치기 쉬운 부분이라.
eslint나 editconfig 등을 활용하고 있는데요.

https://github.com/mash-up-kr-web/github_star_9/pull/2/files#diff-1e70daafb475c0ce3fef7d2728279182

요 링크에 적절한 사용 예시가 있는 듯 해 첨부합니다.

eastroots92 · 2020-04-16T02:43:15Z

@@ -0,0 +1,3 @@
+import axios from 'axios';
+
+export const getRepo = (username: string) => axios.get(`https://api.github.com/users/${username}/repos`);


요기도 함께 챙겨주세요!

eastroots92 · 2020-04-16T02:59:40Z

+  return (
+    <ul className={cx('repo-list')}>
+      {
+        repos.map((repo, index) => <RepoItem key={index} repo={repo} />)


오 희라님 의견 동의합니다.
추가로 index params 또한 생략 가능하겠네요!

Suggested change

repos.map((repo, index) => <RepoItem key={index} repo={repo} />)

repos.map((repo) => <RepoItem key={repo.id} repo={repo} />)

eastroots92 · 2020-04-16T03:00:10Z

+  return (
+    <Loading loading={loading}>
+      {
+        () => (
+          <MainComponent
+            repos={repos}
+            username={username}
+            onChangeInput={handleChangeInput}
+            onSearch={handleSearch}
+          />
+        )
+      }
+    </Loading>
+  );


저도 궁금합니다.

eastroots92 · 2020-04-16T03:01:19Z

@@ -0,0 +1,6 @@
+body {
+  width: 720px;


body의 width를 강제하신 이유가 있을까요??

@eastroots92
body를 중앙으로 정렬하고싶은데 크기가 있어야 될거같아서 명시적으로 width값을 주게 되었습니다!

eastroots92 · 2020-04-16T03:01:47Z

+  width: 720px;
+  margin: 0;
+  padding: 0;
+  margin: 0 auto;


위의 margin과 중첩되어 있는데 확인해주시면 좋을 것 같습니다!

엇 그러네요! 감사합니다!

eastroots92 · 2020-04-16T03:06:27Z

+  name: string;
+  stargazers_count: number;
+}
+export interface Repo {


한 파일에 acion 정의와 reducer 등등이 모두 관리되고 있는데,
서비스의 사이즈가 작아서 하나로 관리하고 있는 것 일까요?

ducks 구조를 사용하신건 아닐까요?
http://guswnsxodlf.github.io/redux-ducks-pattern

@eastroots92
액션, 액선생성함수, 리듀서는 계속 번갈아 봐야되는 경우가 많아 한 파일에 함께 써주는게 편하다고 느꼈습니다!
ducks구조 사용했습니다

아항 그런듯하네요.
감사합니다.

만약 Ducks 구조로 가져간다면, 도메인 관심사(?) 별로 폴더구조를 가져가도 좋을 것 같아요!

eastroots92 · 2020-04-16T03:11:07Z

+const INITIALIZE = 'repo/INITIALIZE' as const;
+const GET_REPO = 'repo/GET_REPO' as const;
+const GET_REPO_LOADING = 'repo/GET_REPO_LOADING' as const;
+const GET_REPO_SUCCESS = 'repo/GET_REPO_SUCCESS' as const;
+const GET_REPO_FAILURE = 'repo/GET_REPO_FAILURE' as const;
+
+export const initalize = () => ({ type: INITIALIZE });
+export const getRepo = (username: string) => ({ type: GET_REPO, payload: username });
+const getRepoLoading = () => ({ type: GET_REPO_LOADING });
+const getRepoSuccess = (data: ResponseRepo[]) => ({ type: GET_REPO_SUCCESS, payload: data });
+const getRepoFailure = () => ({ type: GET_REPO_FAILURE });


정말 소소한 이야기이긴 한데

redux를 사용하면서 size가 커지면 요런 행사코드들이 많이 생기더라구요.

그래서 저는 해당 내용들을 중복되는 부분만 빼서 제네레이터 함수로 만들어 사용하는 편이에요.

아니면 redux-toolkit도 한번 사용해보시면서 장단을 비교해 보시는 것도 추천드립니다.

저도 js쓸땐 redux-actions 쓰면 코드가 깔끔하고 좋았는데 ts쓰게 되면서 redux-actions이 ts지원을 제대로 안해줘서 redux-saga하고 쓰면 코드가 길어지더라구요.. 말씀해주신 redux-toolkit 한번 찾아보겠습니다!

rdd9223 · 2020-04-16T06:11:19Z

+const MainComponent: React.FC<Props> = ({ repos, username, onChangeInput, onSearch}) => {
+  const starCount = repos.reduce((p, c) => p + c.star, 0);
+  return (
+    <>


여기에 <>태그의 기능이 궁금해요! 첨보는것같아요 ㅎㅎ

Fragment라고 별도의 DOM 추가없이 하위 요소들을 그룹화 할수 있습니다!

https://ko.reactjs.org/docs/fragments.html

네 예전에는 <React.Fragment> 로 적어줘야 했는데 리액트 몇버전부터인가 <>이렇게 짧게 사용할 수 있도록 변경되었습니다.

yurim22 · 2020-04-16T10:10:19Z

+        </form>
+      </header>
+      <section>
+        <article className={cx('info')}>


section이나 article 같은 시멘틱 태그를 활용해서 코드를 보기 쉽게 하신건 좋은 것 같아요!😊

에전에는 전부다 div 로 때려박았는데 점점 시멘틱웹쪽으로 가도록 노력하고 있습니다!

Yuni-Q · 2020-04-16T14:32:39Z

+import Loading from '../components/Loading';
+
+const MainContainer: React.FC = () => {
+  const [username, setUsername] = useState<string>('');


제네릭을 string으로 명시한 이유가 있을까요?

Yuni-Q · 2020-04-16T14:36:08Z

+import { all } from 'redux-saga/effects';
+import repo, { repoSaga } from './repo';
+
+const rootReducer = combineReducers({


reduce가 하나인데 combineReducers를 사용하신게 나중에 있을 확정성을 대비하신 걸까요?

넵 사실 과제다 보니 확장될 가능성은 거의없긴 하겠지만 주로 프로젝트에 combineReducers를 쓰다보니 습관적으로 쓰게 된거같아요! 리듀서 하나이니 빼주는게 나을거같네요

joi0104 · 2020-04-16T07:06:05Z

+    margin-bottom: 10px;
+    font-size: 2.4rem;


잘 몰라서 그러는데 px와 rem을 혼용해서 쓰신 이유가 있을까요?

px은 절댓값으로 크기가 정해지는데 rem은 dom의 최상위(body)의 크기에 영향을 받아 주로 font-size속성은 rem이나 em을 쓰게 됐습니다! 사실 저도 css를 잘다루지 못하기 때문에 더 효과적인 css방법론이 있을거같지만 제 한계네요..ㅠㅠ

kand193 · 2020-04-17T15:28:53Z

+ReturnType<typeof getRepo> |
+ReturnType<typeof getRepoLoading> |
+ReturnType<typeof getRepoSuccess> |
+ReturnType<typeof getRepoFailure>


섬세한 타입정의 👍 👍

danivelop added 2 commits March 19, 2020 15:42

homework(get github repository list by user name)

0427292

remove star api(misunderstood)

98d9709

lallaheeee reviewed Apr 15, 2020

View reviewed changes

jieunjieun reviewed Apr 15, 2020

View reviewed changes

YukJiSoo reviewed Apr 15, 2020

View reviewed changes

mango906 reviewed Apr 16, 2020

View reviewed changes

eastroots92 reviewed Apr 16, 2020

View reviewed changes

rdd9223 reviewed Apr 16, 2020

View reviewed changes

yurim22 reviewed Apr 16, 2020

View reviewed changes

Yuni-Q reviewed Apr 16, 2020

View reviewed changes

joi0104 reviewed Apr 16, 2020

View reviewed changes

kand193 reviewed Apr 17, 2020

View reviewed changes

		@@ -0,0 +1 @@
		export { default } from './Loading'; No newline at end of file

	repos.map((repo, index) => <RepoItem key={index} repo={repo} />)
	repos.map(({id, ...repo }, index) => <RepoItem key={id} repo={repo} />)

	repos.map((repo, index) => <RepoItem key={index} repo={repo} />)
	repos.map((repo) => <RepoItem key={repo.id} repo={repo} />)

		@@ -0,0 +1,23 @@
		# See https://help.github.com/articles/ignoring-files/ for more about ignoring files.

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

		export const getRepo = (username: string) => axios.get(`https://api.github.com/users/${username}/repos`); No newline at end of file

Uh oh!

Conversation

danivelop commented Mar 19, 2020

Uh oh!

lallaheeee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eastroots92 Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YukJiSoo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yuni-Q Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eastroots92 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eastroots92 Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eastroots92 Apr 16, 2020 •

edited

Loading

YukJiSoo left a comment •

edited

Loading

Yuni-Q Apr 16, 2020 •

edited

Loading

eastroots92 Apr 16, 2020 •

edited

Loading