github_start_9 최진영 과제 제출합니다. #2

joi0104 · 2020-04-01T07:58:04Z

vue 환경에서 개발하였습니다.

실행은 node가 깔린 환경에서 npm install로 setup을 해주신 뒤에,

npm run serve를 통해 실행하시면 됩니다!

vue의 렌더링 매커니즘이나 반응형에 대한 지식이 미흡하여 속도가 조금 느린 결함이 존재합니다 ㅠ

This reverts commit 6d913e0.

lallaheeee

뷰는 정말 신기한 거같아요! 덕분에 뷰 코드도 읽어보네요 ㅎㅎ 고생하셨습니다 🤩

lallaheeee · 2020-04-15T07:09:26Z

src/components/RepoList.vue

+        .then((res) => {
+          this.isFetchSuccess = true;
+          this.repoNameList = [];
+          res.data.forEach((element) => {
+            this.repoNameList.push(element.name);
+          });
+        })


제가 vue는 잘 모르지만 아래는 어떠나유?! ㅎㅎ

Suggested change

.then((res) => {

this.isFetchSuccess = true;

this.repoNameList = [];

res.data.forEach((element) => {

this.repoNameList.push(element.name);

});

})

.then(({data: repos}) => {

this.isFetchSuccess = true;

this.repoNameList = repos.map(repo => repo.name);

})

lallaheeee · 2020-04-15T07:27:22Z

package.json

+  "eslintConfig": {
+    "root": true,
+    "env": {
+      "node": true


Suggested change

"node": true

"browser": true

가 되어야할 것 같아요! ㅎㅎ

오오 감사합니다!

lallaheeee · 2020-04-15T07:30:54Z

src/components/RepoList.vue

+      <p>{{ totalRepoStarCount }} stars</p>
+    </div>
+    <hr>
+    <p v-if="isEmptyRepoList">


Suggested change





사소한건데 이렇게 바꾸면 isEmptyRepoList를 줄일 수 있을 것 같아요! ㅎㅎ

오 감사합니다!

lallaheeee · 2020-04-15T07:32:24Z

src/components/RepoList.vue

+        .then((res) => {
+          this.repoList = res.data;


response가 꽤 길던데 메모리 사용을 줄이기 위해 필요한 데이터만 한번 추출하면 어떨까요?! ㅎㅎ

메모리는 미쳐 생각하지 못했네요 ㅎㅎ 감사합니다!

lallaheeee · 2020-04-15T07:36:13Z

src/components/RepoList.vue

+    <repo-list-item
+      v-else
+      v-for="(repoListItem,idx) in repoList"
+      :key="idx"


Suggested change

:key="idx"

:key="repoListItem.id"

는 어떠신가요?! ㅎㅎ

https://kr.vuejs.org/v2/guide/list.html#Maintaining-State

lallaheeee · 2020-04-15T07:37:15Z

src/components/RepoList.vue

+#repoList {
+  width: 500px;
+}
+#totalCount p{


스타일 설정을 위해 id를 부여하고 사용하신 이유가 있을까요?!

id로 접근해서 스타일을 설정하는 방식이 편해서 그런식으로 진행했었는데요. 다른 좋은 방법이 있을까요? 안그래도 id 지을때마다 힘들었었거든요 ㅎㅎ

찾아보니 vue에도 styled-components가 있네요! 사용해보셔도 괜찮을듯해요

lallaheeee · 2020-04-15T07:39:36Z

src/components/Result.vue

+  },
+  methods: {
+    fecthUserName() {
+      this.$http.get(`https://api.github.com/users/${this.userName}`)


username을 한번 검색한 후에 repo를 검색하는 이유가 있을까요?! !

유효한 username인지 확인하는 과정을 거쳤습니다. 검색한 username이 git 유저에 등록되어있지 않다면, 유효하지 않는 name이라는 안내창을 띄워줍니다!

섬세하네요!

NuclearPunch · 2020-04-15T12:28:54Z

src/components/RepoList.vue

+      <p>{{ totalRepoCount }} repositories</p>
+      <p>|</p>
+      <p>{{ totalRepoStarCount }} stars</p>


p태그마다 margin을 주기위한 의도였다면 상위 div를 flex로 주시고 Template Literal로 감싸줘도 좋을 것 같습니다 👍
#totalCount { display: flex; justify-content: space-between; //space-around; }

Suggested change

{{ totalRepoCount }} repositories

|

{{ totalRepoStarCount }} stars

${ totalRepoCount } repositories | ${ totalRepoStarCount } star

헛 그러네요 ㅎㅎ 왜 저땐 저생각을 못했을까요...? 감사합니다 ㅎㅎ

mango906 · 2020-04-15T12:34:37Z

src/components/RepoList.vue

+    totalRepoStarCount() {
+      return this.repoList
+        .map((element) => element.stargazers_count, [])
+        .reduce((x, y) => x + y, 0);
+    },


이런식으로 간소화 할수 있을거 같습니다!

Suggested change

totalRepoStarCount() {

return this.repoList

.map((element) => element.stargazers_count, [])

.reduce((x, y) => x + y, 0);

},

totalRepoStarCount() {

return this.repoList

.reduce((x, y) => x + y.stargazers_count, 0);

},

return this.repoList .reduce((accumulator, currentValue) => accumulator + currentValue.stargazers_count, 0);

이런식으로 수정하도록 하겠습니다! 감사합니다 ㅎㅎ

YukJiSoo

자바스크립트도 알기전에 처음 접한게 vue였는데 이렇게 다시 vue 코드를 보게 될 줄 몰랐어요ㅋㅋㅋ
재밌었어요ㅎㅎ

YukJiSoo · 2020-04-15T19:04:48Z

package.json

@@ -0,0 +1,83 @@
+{


package.json에 eslint, browseList, jest 관련 설정이 모두 포함되어 있는 것을
별도의 파일로 분리하면 어떨까요?

설정파일도 목적별로 분리하면 유지보수하기도 좋고 관심사 분리도 되어서 좋다고 생각해요!

저도 동의합니다. 👍

오오 그러네요... 참고하도록 하겠습니다!

YukJiSoo · 2020-04-15T19:13:47Z

src/components/Search.vue

+  },
+  methods: {
+    onSubmit(evt) {
+      this.$router.push(`/${this.userName}`);


username 검색을 path에 추가해서 routing하는 식으로 구현하신 이유가 있나요?? 궁금해요!

이번에 네스티드 라우터를 써보고 싶어서 path를 추가하는 방식으로 구현을 해봤습니다 ㅎㅎ

YukJiSoo · 2020-04-15T19:18:10Z

src/main.js

+new Vue({
+  router,
+  store,
+  render: (h) => h(App),


h는 어떤 의미가 있는건가요?? convention 같은 건가요??

저도 궁금합니다!

이 부분은 처음에 뷰 프로젝트를 진행할 때 필요한 설정들을 자동으로 해주는 vue-cli가 자동으로 생성해줬던 부분이라 ㅎㅎ 위 코드에 대해서 자세히 보지는 못했네요 ㅎㅎ 저도 궁금해지는데.. 찾아보고 다시 리뷰 달도록 하겠습니다!

eastroots92

뷰 신기하네요

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

package.json

@@ -0,0 +1,83 @@
+{


저도 동의합니다. 👍

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

.editorconfig

+[*.{js,jsx,ts,tsx,vue}]
+indent_style = space
+indent_size = 2
+end_of_line = lf
+trim_trailing_whitespace = true
+insert_final_newline = true
+max_line_length = 100


위에 코드와 중복되는 것 같은데 의도하신 것이 맞을까요?

엇.. 그러네요! 수정하도록 하겠습니다!

eastroots92 · 2020-04-16T02:17:06Z

src/components/RepoList.vue

@@ -0,0 +1,84 @@
+<template>
+  <div id="repoList">


Suggested change

<div id="repoList">

<article id="repoList">

사실 vue 알 못 이라.
하이라키가 명확히 그려지진 않아, 적절한 semantic tag 부여인진 모르겠는데,

PC 페이지로 만들어지는 만큼 semantic tag들을 챙겨주시면 더 좋을 것 같습니다.

그러네요! 개발하면서 시멘틱태그를 미쳐 신경쓰지 못했네요 감사합니다!

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

src/components/RepoList.vue

+    totalRepoStarCount() {
+      return this.repoList
+        .map((element) => element.stargazers_count, [])
+        .reduce((x, y) => x + y, 0);


정말 사소한 개인 피드백인데
reduce의 경우엔 js에서 기본적으로 내장된 함수여서 다른 분들이 쉽게 params 값들을 유추 할 수있을 듯 하나,
그럼에도 불구하고 최소한의 의미 부여를 하는 것은 어떻게 생각하세요?

Suggested change

.reduce((x, y) => x + y, 0);

.reduce((accumulator, currentValue) => accumulator + currentValue, 0);

좋은 방법이라고 생각해요!

좋은거 같아요! 수정하도록 하겠습니다 ㅎㅎ

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

src/components/Search.vue

+};
+</script>
+
+<style scoped>


lint 룰에서 space 2 였던 것 같은데

해당 부분에 space 4로 되어있는 것 같습니다! 의도하신 것이 맞을까요?

그러네요! css indent를 신경못쓰고 있었네요! 탭을 누르는 습관때문에.. 이렇게 습관이 무섭네요.. 수정하도록 하겠습니다 ㅎㅎ

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

src/main.js

+new Vue({
+  router,
+  store,
+  render: (h) => h(App),


저도 궁금합니다!

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

src/components/RepoListItem.vue

+#repoStar{
+  display: inline-block;
+  width: 10%;
+  font-size: 20px;


em과 px가 혼재되어있는데
어떤 기준으로 구분하여 사용하고 있는지 알 수 있을까요?

이 부분은 수정하다가 못바꿨던 부분이었네요 ㅎㅎ 의도했던건 px와 %만 사용하려고 했습니다 ㅎㅎ 수정하도록 할께요!

rdd9223

뷰.js 는 처음봐서 정말 신기했어요 ㅋㅋㅋ
새로운 접근이라 재밌었습니다!!!

Yuni-Q · 2020-04-16T14:44:28Z

public/index.html

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


동근님이 다른 리뷰에서 말씀하셨던 부분인긴 한데 여기 lang를 ko로 바꾸면 좋을거 같아요 ㅎㅎ

미쳐 신경 못쓴 부분이네요 감사합니다 ㅎㅎ

joi0104 added 18 commits March 23, 2020 14:34

Initialize commit

d25d3af

Create search,result component in home

dad4c12

Create search component ui

eac6b79

Add axios plugin

ed55ef2

Add set and get username in store

6d913e0

Revert "Add set and get username in store"

de7689b

This reverts commit 6d913e0.

Add isEmptyUserName and isVaildUserName

348013a

Add repoList if notEmpty and vaild userName

bdd998a

Refactoring search and result ui

cdcb5ed

Create repolist ui

fed01f2

Remove duplication of http

fdc114e

Create RepoListItem

a2f595e

Use nested router

78e4585

Refactoring all

87edb09

Add totalRepoCount and totalRepoStarCount

dc56a28

Upgrade stylesheet

8fad5ba

Upgrade repoListItem stylesheet

add9e3e

Upgrade repoListItem stylesheet

ccbf0a7

lallaheeee reviewed Apr 15, 2020

View reviewed changes

NuclearPunch reviewed Apr 15, 2020

View reviewed changes

mango906 reviewed Apr 15, 2020

View reviewed changes

YukJiSoo reviewed Apr 15, 2020

View reviewed changes

eastroots92 reviewed Apr 16, 2020

View reviewed changes

rdd9223 reviewed Apr 16, 2020

View reviewed changes

Yuni-Q reviewed Apr 16, 2020

View reviewed changes

	.reduce((x, y) => x + y, 0);
	.reduce((accumulator, currentValue) => accumulator + currentValue, 0);

	<p v-if="isEmptyRepoList">
	<p v-if="!totalRepoCount">

github_start_9 최진영 과제 제출합니다. #2

Are you sure you want to change the base?

github_start_9 최진영 과제 제출합니다. #2

Conversation

joi0104 commented Apr 1, 2020

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

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

YukJiSoo 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

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

rdd9223 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment