github_star_9 박영진 과제 제출합니다. #5

kand193 · 2020-04-05T10:50:14Z

React를 사용해서 과제를 구현하였습니다.
패키지 설치 후 start 스크립트를 통해서 시작하시면 1234 포트에서 확인 가능합니다.

jieunjieun · 2020-04-15T11:51:27Z

src/contexts/home/components/RepoResult.tsx

+            {repos.length}
+            &nbsp;
+            {repos.length > 1 ? "repositories" : "repository"}
+            &nbsp;|&nbsp;


호옥시... &nbsp를 사용하신 이유가 따로 있을까용?
저 같은 경우에는 nbsp를 사용하게 되면 뭔가.. 협업할때 불편하다는 피드백을 받았어서 사용하지 않고 있었는데,
특별히 사용하신 이유가 있을까용?

음.. 저는 딱히 nbsp에 대한 피드백을 따로 들었던 적이 없어서 계속 사용을 하고 있었는데요
개인적으로 {" "}를 안쓰는 편이라 사용하고 있었어요.
nbsp에 관한 의견들을 찾아봐야겠네요! 의견 감사합니다.

mango906 · 2020-04-15T12:01:28Z

src/contexts/home/components/Title.tsx

+  const handleChange = useCallback(
+    (e: React.ChangeEvent<HTMLInputElement>) => {
+      setInputValue(e?.target?.value);
+    },
+    [setInputValue],
+  );


e.target.value를 Optional Chaining으로 선언하신 이유가 있을까요???

요것 저도 궁금합니다.
뭔가 항상 있을 듯 한데 👀

mango906 · 2020-04-15T12:03:10Z

src/services/GithubService.ts

+  public async getRepos(username: string) {
+    const res =
+      (await this.getReposFromOrg(username)) ||
+      (await this.getReposFromUsername(username));
+
+    return res ? res?.data.map((repo: RepoResult) => ({
+      name: repo.name ?? "untitled",
+      url: repo.html_url,
+      stargazersCount: repo.stargazers_count,
+    })) as Repository[] : [];
+  }


Suggested change

public async getRepos(username: string) {

const res =

(await this.getReposFromOrg(username)) ||

(await this.getReposFromUsername(username));

return res ? res?.data.map((repo: RepoResult) => ({

name: repo.name ?? "untitled",

url: repo.html_url,

stargazersCount: repo.stargazers_count,

})) as Repository[] : [];

}

public async getRepos(username: string) {

const res =

(await this.getReposFromOrg(username)) ||

(await this.getReposFromUsername(username));

return res?.data.map((repo: RepoResult) => ({

name: repo.name ?? "untitled",

url: repo.html_url,

stargazersCount: repo.stargazers_count,

})) as Repository[];

}

이렇게 수정하면 어떨까요??

mango906 · 2020-04-15T12:05:20Z

@types/response.d.ts

+declare type RepoResult = {
+  archive_url: string;
+  archived: boolean;
+  assignees_url: string;
+  blobs_url: string;
+  branches_url: string;
+  clone_url: string;
+  collaborators_url: string;
+  comments_url: string;
+  commits_url: string;
+  compare_url: string;
+  contents_url: string;
+  contributors_url: string;
+  created_at: string;
+  default_branch: string;
+  deployments_url: string;
+  description: string;
+  disabled: boolean;
+  downloads_url: string;
+  events_url: string;
+  fork: boolean;
+  forks_count: number;
+  forks_url: string;
+  full_name: string;
+  git_commits_url: string;
+  git_refs_url: string;
+  git_tags_url: string;
+  git_url: string;
+  has_downloads: boolean;
+  has_issues: boolean;
+  has_pages: boolean;
+  has_projects: boolean;
+  has_wiki: boolean;
+  homepage: string;
+  hooks_url: string;
+  html_url: string;
+  id: number;
+  is_template: boolean;
+  issue_comment_url: string;
+  issue_events_url: string;
+  issues_url: string;
+  keys_url: string;
+  labels_url: string;
+  language: null;
+  languages_url: string;
+  license: any;
+  merges_url: string;
+  milestones_url: string;
+  mirror_url: string;
+  name: string;
+  network_count: number;
+  node_id: string;
+  notifications_url: string;
+  open_issues_count: number;
+  owner: any;
+  permissions: any;
+  private: boolean;
+  pulls_url: string;
+  pushed_at: string;
+  releases_url: string;
+  size: number;
+  ssh_url: string;
+  stargazers_count: number;
+  stargazers_url: string;
+  statuses_url: string;
+  subscribers_count: number;
+  subscribers_url: string;
+  subscription_url: string;
+  svn_url: string;
+  tags_url: string;
+  teams_url: string;
+  temp_clone_token: string;
+  template_repository: null;
+  topics: Array<string>;
+  trees_url: string;
+  updated_at: string;
+  url: string;
+  visibility: string;
+  watchers_count: number;
+};


저도 결과값에 대한 type을 이렇게 전부 정의할까 고민했는데, 필요한 값들만 정의한다면 어떨까요??

여기에는 api 응답값을 모두 적고 필요한 값만 따로 utils/model 내에 따로 정의를 해놓긴 했었는데 불필요한 정보가 많긴 해서 정리하면 좋아보이네요!

요것 보면서 저도 꼭 필요한 타입만 정의해서 쓰는 것이 어떨까 생각이 들었습니다.

사용하지 않는 내용들의 Type들은 추후 개발할 때 혼란스러움이 있을 수 있고, 휴먼에러를 발생하기 쉬울 듯 해서요!

mango906 · 2020-04-15T12:06:27Z

src/contexts/home/HomeContext.tsx

+  const { children } = props;
+
+  const [username, setUsername] = useState("");
+  const [repos, setRepos] = useState([] as Repository[]);


이렇게 제네릭 형식으로 선언하는건 어떨까요??

Suggested change

const [repos, setRepos] = useState([] as Repository[]);

const [repos, setRepos] = useState<Repository[]>([]);

as를 뒤늦게 처리해서 놓치고 있었네요 ㅋㅋ

오 제네릭으로 처리할 수 도 있군요. 처음 알았습니다 👍

mango906 · 2020-04-15T12:13:07Z

src/contexts/home/components/RepoItem.tsx

+
+const RepoItem = ({ repo }: RepoItemProps) => {
+  const handleColumnClick = useCallback(
+    (url?: string) => () => {


이부분도 굳이 optional chaining으로 선언할 필요 없다고 생각합니다!

YukJiSoo · 2020-04-15T16:36:49Z

package.json

+  "license": "MIT",
+  "private": true,
+  "scripts": {
+    "start": "parcel src/index.html"


오 parcel을 사용하셨네요 😮
parcel을 써보시니 어떤점이 좋은가요??

저도 궁금합니다!

parcel의 장점이 많은 설정이 필요없다는 점인데요, 제가 여기서 사용했던 타입스크립트와 리액트는 기본적으로 parcel에서 지원을 하고 있어요.
그래서 번들에 대한 설정 필요없이 간단하게 사용할 수 있어서 사용했어요!

오오 저도 한번 써보고 싶어요

YukJiSoo · 2020-04-15T17:27:34Z

src/contexts/home/HomeContext.tsx

+  const searchRepo = useCallback(async (name: string) => {
+    const res = await githubService.getRepos(name);
+    setUsername(name);
+    setRepos(res);
+    setTotalStars(res.reduce((p, c) => p + (c.stargazersCount ?? 0), 0));
+  }, [username]);


searchRepo의 dependency list에 username이 없으면 안될까요??

아 중간에 username이 수정된 것을 놓치고 지우지 못했었네요! 감사합니다

YukJiSoo · 2020-04-15T17:38:09Z

src/services/GithubService.ts

@@ -0,0 +1,47 @@
+import { Octokit } from "@octokit/rest";


오 이런 모듈도 있네요! 👍👍

완전 신기하네요 ㅎㅎ

와 ㅋㅋㅋㅋ 이런게 있는지 첨알았네요 ㅋㅋㅋ

joi0104 · 2020-04-16T07:47:10Z

src/contexts/home/components/RepoResult.tsx

+      ) : (
+        <h3>결과가 없습니다.</h3>
+      )}


입력한 유저 아이디가 유효한 아이디가 아닙니다. 유저의 레포지토리가 존재하지 않습니다. 처럼 결과가 없을 시 자세한 안내문구를 띄워주는 것도 좋을 것 같아요!

UX를 편하게 만들 수 있는 방법을 더 고민해봐야겠네요!

lallaheeee

코드가 깔끔해서 읽기 좋았어요 !! 👍🏻 고생하셨습니다

lallaheeee · 2020-04-16T10:09:23Z

package.json

+  "license": "MIT",
+  "private": true,
+  "scripts": {
+    "start": "parcel src/index.html"


오오 저도 한번 써보고 싶어요

lallaheeee · 2020-04-16T10:11:40Z

src/contexts/home/components/RepoResult.tsx

+          <h3>
+            {repos.length}
+            &nbsp;
+            {repos.length > 1 ? "repositories" : "repository"}


섬세합니다 👍🏻

lallaheeee · 2020-04-16T10:12:49Z

src/services/GithubService.ts

@@ -0,0 +1,47 @@
+import { Octokit } from "@octokit/rest";


완전 신기하네요 ㅎㅎ

Yuni-Q · 2020-04-16T16:42:09Z

src/App.tsx

+  <Router>
+    <Switch>
+      <Route path="/">
+        <Home />


페이지가 하나인데 react-router-dom 쓴 이유가 궁금합니다 !!

버릇처럼 프로젝트 생성할때마다 router를 붙이게 되더라구요 ㅋㅋ

Yuni-Q · 2020-04-16T16:44:42Z

src/contexts/home/HomeContext.tsx

+    const res = await githubService.getRepos(name);
+    setUsername(name);
+    setRepos(res);
+    setTotalStars(res.reduce((p, c) => p + (c.stargazersCount ?? 0), 0));


reduce를 따로 분리하면 가독성이 더 올라갈거 같습니다 !!

Yuni-Q · 2020-04-16T16:45:06Z

src/contexts/home/HomeContext.tsx

+  const githubService = new GithubService();
+
+  const searchRepo = useCallback(async (name: string) => {
+    const res = await githubService.getRepos(name);


이름을 좀더 명시적으로 작성할수 있을거 같습니다 !

danivelop · 2020-04-16T17:36:07Z

src/contexts/home/components/RepoResult.tsx

+            {repos.map((repo: Repository) => (
+              <RepoItem repo={repo} key={repo.name} />
+            ))}


혹시 RepoItem컴포넌트의 key값으로 준 repo.name 이 고유한 값인가요?? 혹시 중복되는 경우가 있다면 렌더링에 문제가 생길수도 있어 repo.id 가 더 좋을거 같다는 생각이 들었습니다!

id가 더 괜찮겠네요!

danivelop · 2020-04-16T17:40:55Z

src/contexts/home/components/Title.tsx

+      <TitleInput value={inputValue} onChange={handleChange} onKeyDown={handleEnter} />
+      <TitleButton onClick={handleSearch}>Search</TitleButton>


이부분을 form 태그를 사용해 작성해보는건 어떨까요? 위에서 inputValue가 변경될 때마다 매번 handleSearch와 handleEnter 2개의 새로운함수가 할당되는데 form태그를 사용할 경우 이 횟수를 반으로 줄일수 있어 렌더링에 조금더 효과적일수 있을거 같아요

아 form을 생각 못하고 있었네요! 감사합니다 👍

eastroots92

와.. 👍

eastroots92 · 2020-04-17T12:22:54Z

@types/response.d.ts

@@ -0,0 +1,80 @@
+declare type RepoResult = {


와... 이거 혹시 Type 만들어주는 제네레이터가 있나요????

제네레이터는 없고 결과값을 가왔어요!

eastroots92 · 2020-04-17T12:23:58Z

@types/response.d.ts

+declare type RepoResult = {
+  archive_url: string;
+  archived: boolean;
+  assignees_url: string;
+  blobs_url: string;
+  branches_url: string;
+  clone_url: string;
+  collaborators_url: string;
+  comments_url: string;
+  commits_url: string;
+  compare_url: string;
+  contents_url: string;
+  contributors_url: string;
+  created_at: string;
+  default_branch: string;
+  deployments_url: string;
+  description: string;
+  disabled: boolean;
+  downloads_url: string;
+  events_url: string;
+  fork: boolean;
+  forks_count: number;
+  forks_url: string;
+  full_name: string;
+  git_commits_url: string;
+  git_refs_url: string;
+  git_tags_url: string;
+  git_url: string;
+  has_downloads: boolean;
+  has_issues: boolean;
+  has_pages: boolean;
+  has_projects: boolean;
+  has_wiki: boolean;
+  homepage: string;
+  hooks_url: string;
+  html_url: string;
+  id: number;
+  is_template: boolean;
+  issue_comment_url: string;
+  issue_events_url: string;
+  issues_url: string;
+  keys_url: string;
+  labels_url: string;
+  language: null;
+  languages_url: string;
+  license: any;
+  merges_url: string;
+  milestones_url: string;
+  mirror_url: string;
+  name: string;
+  network_count: number;
+  node_id: string;
+  notifications_url: string;
+  open_issues_count: number;
+  owner: any;
+  permissions: any;
+  private: boolean;
+  pulls_url: string;
+  pushed_at: string;
+  releases_url: string;
+  size: number;
+  ssh_url: string;
+  stargazers_count: number;
+  stargazers_url: string;
+  statuses_url: string;
+  subscribers_count: number;
+  subscribers_url: string;
+  subscription_url: string;
+  svn_url: string;
+  tags_url: string;
+  teams_url: string;
+  temp_clone_token: string;
+  template_repository: null;
+  topics: Array<string>;
+  trees_url: string;
+  updated_at: string;
+  url: string;
+  visibility: string;
+  watchers_count: number;
+};


요것 보면서 저도 꼭 필요한 타입만 정의해서 쓰는 것이 어떨까 생각이 들었습니다.

사용하지 않는 내용들의 Type들은 추후 개발할 때 혼란스러움이 있을 수 있고, 휴먼에러를 발생하기 쉬울 듯 해서요!

eastroots92 · 2020-04-17T12:24:24Z

package.json

+  "license": "MIT",
+  "private": true,
+  "scripts": {
+    "start": "parcel src/index.html"


eastroots92 · 2020-04-17T12:27:31Z

src/App.tsx

+import React from "react";
+import { Route, Switch, BrowserRouter as Router } from "react-router-dom";
+
+import Home from "~/contexts/home";


오 alias 👍

eastroots92 · 2020-04-17T12:30:13Z

src/contexts/home/HomeContext.tsx

+  const { children } = props;
+
+  const [username, setUsername] = useState("");
+  const [repos, setRepos] = useState([] as Repository[]);


오 제네릭으로 처리할 수 도 있군요. 처음 알았습니다 👍

eastroots92 · 2020-04-17T12:33:21Z

src/contexts/home/components/RepoResult.tsx

+const RepoList = styled.ul`
+  list-style: none;
+  padding: 0;
+  margin: 10px 0 0 0;


nit 하긴한데 요렇게 생략도 가능합니다 !

Suggested change

margin: 10px 0 0 0;

margin: 10px 0 0;

eastroots92 · 2020-04-17T12:33:55Z

src/contexts/home/components/Title.tsx

+  const handleChange = useCallback(
+    (e: React.ChangeEvent<HTMLInputElement>) => {
+      setInputValue(e?.target?.value);
+    },
+    [setInputValue],
+  );


요것 저도 궁금합니다.
뭔가 항상 있을 듯 한데 👀

eastroots92 · 2020-04-17T12:34:20Z

src/index.html

+    <div id="root" />
+    <script src="./index.tsx"></script>
+  </body>
+</html>


하단에 empty line 추가해주시면 좋을 것 같아요!

eastroots92 · 2020-04-17T12:35:10Z

src/services/GithubService.ts

@@ -0,0 +1,47 @@
+import { Octokit } from "@octokit/rest";


와 ㅋㅋㅋㅋ 이런게 있는지 첨알았네요 ㅋㅋㅋ

kand193 added 6 commits March 19, 2020 18:23

chore: Initial setting for React

ae203f4

chore: Apply eslint setting and resolve module

dddb781

feat: Implement Title component and store

c2f500a

feat: Implement getting repos from Github API

9c5c9ec

feat: Implement displaying result component

9ddf998

fix: Remove a unused function

1d21ad2

jieunjieun reviewed Apr 15, 2020

View reviewed changes

mango906 reviewed Apr 15, 2020

View reviewed changes

YukJiSoo reviewed Apr 15, 2020

View reviewed changes

joi0104 reviewed Apr 16, 2020

View reviewed changes

lallaheeee reviewed Apr 16, 2020

View reviewed changes

Yuni-Q reviewed Apr 16, 2020

View reviewed changes

danivelop reviewed Apr 16, 2020

View reviewed changes

eastroots92 reviewed Apr 17, 2020

View reviewed changes

	const [repos, setRepos] = useState([] as Repository[]);
	const [repos, setRepos] = useState<Repository[]>([]);

		<TitleInput value={inputValue} onChange={handleChange} onKeyDown={handleEnter} />
		<TitleButton onClick={handleSearch}>Search</TitleButton>

github_star_9 박영진 과제 제출합니다. #5

Are you sure you want to change the base?

github_star_9 박영진 과제 제출합니다. #5

Conversation

kand193 commented Apr 5, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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 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