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_start_9 박정후 과제 제출합니다 #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NuclearPunch
Copy link

바닐라스크립트로 개발하였습니다.
npm install로 http-server 설치하시고
npm run start를 통해 실행하시면 됩니다!
오랜만에 바닐라스크립트를 사용하다보니 생각보다 시간이 오래걸렸네요 :(

App.prototype.render = function(data) {
this.list.render(data);
};

Copy link
Collaborator

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.

감사합니다 ! 힘든 도전이였습니다 ㅋㅋㅋ 아쉬운 부분도 많았어요 ㅜㅜ

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.

바닐라 멋있어요!
makeComponent, render 같은 특정 역할을 하는 함수를 작성해서 component 로직을 작성하신거 너무 좋아요!!👍👍

Comment on lines +5 to +6
const $paragraph = document.createElement("p");
$paragraph.innerText = this.content;

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.

엇... 그렇네요! 전역에서 render로 옮겨가는 과정에서 남아있었나봐요 ㅜㅜ

Comment on lines +19 to +21
}
});

Choose a reason for hiding this comment

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

이 부분 indent가 고장난거 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

앜 ㅜㅜ 이때 prettier가 먹통이여서 그냥 진행했는데 prettier 없이 못살겠어요 ㅜㅜ

Comment on lines +4 to +6
(function() {
new App($("#app"));
})();

Choose a reason for hiding this comment

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

App 인스턴스 생성 로직을 익명함수로 감싸서 실행되도록 작성해주셨는데
전역에서 실행하면 발생될 수 있는 side effect를 방지하려고 하신건가요??
어떤 이유에서 익명함수로 한번 감싸주셨는지 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

넵! 전역에서의 실행을 피해서 side effect를 방지하고 최초 실행되는 구문이라는걸 명시적으로 표현하기 좋을 것 같아서 사용했습니다 :)

$listLow.classList.add("list-rows");
$title.classList.add("list-title");
$statCnt.classList.add("list-star");
$title.innerText = data.name;
Copy link
Member

Choose a reason for hiding this comment

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

존재하지 않는 user name이라면 존재하지 않는 유저입니다 라는 안내 메시지를 띄워줘도 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

에러핸들링을 고려못했네요! 좋은 포인트!! 감사합니다 👍

Copy link
Member

@lallaheeee lallaheeee left a comment

Choose a reason for hiding this comment

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

바닐라스크립트 짱 멋있어요 👍🏻🤙🏻

@@ -0,0 +1,19 @@
export const $ = selector => document.querySelector(selector);
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.

오 정말 좋은아이디어네요! 👍 희라님 짱짱이십니다

"scripts": {
"start": "npx http-server"
},
"dependencies": {},
Copy link
Member

Choose a reason for hiding this comment

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

혹시 node_modules 을 따로 올리신 이유가 있을까요?!

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.

아 제가 index.html에서 index.js를 모듈로 사용하고 있기때문에 javascript module 보안 요구사항 특성때문에 local에서 실행 시 CORS 오류가 발생됩니다 ㅎㅎ 그래서 이를 방지하고자 http-server에 올렸습니다!
자세한 설명은 아래 블로그에 잘 정리되어있습니다https://velog.io/@takeknowledge/%EB%A1%9C%EC%BB%AC%EC%97%90%EC%84%9C-CORS-policy-%EA%B4%80%EB%A0%A8-%EC%97%90%EB%9F%AC%EA%B0%80-%EB%B0%9C%EC%83%9D%ED%95%98%EB%8A%94-%EC%9D%B4%EC%9C%A0-3gk4gyhreu

Comment on lines +7 to +21
data.map(data => {
const $listLow = document.createElement("div");
const $title = document.createElement("span");
const $statCnt = document.createElement("span");
$listLow.classList.add("list-rows");
$title.classList.add("list-title");
$statCnt.classList.add("list-star");
$title.innerText = data.name;
$statCnt.innerText = data.stargazers_count;

$listLow.addEventListener("click", () => (location.href = data.html_url));
$listLow.appendChild($title);
$listLow.appendChild($statCnt);
this.$listComponent.appendChild($listLow);
});
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.

좋은방법입니다! 코드량도 줄이고 명시적으로 표현 가능할 것 같습니다:)

Copy link

@mango906 mango906 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 +12 to +17
List.prototype.makeComponent = function() {
this.$header = new ListHeader();
this.$body = new ListBody();
this.$searchResultComponent.appendChild(this.$header.$headerComponent);
this.$searchResultComponent.appendChild(this.$body.$listComponent);
};

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.

dom을 갖고있는 변수들앞에 $를 붙여서 구분을 해봤습니다 ㅎㅎ

@rdd9223
Copy link

rdd9223 commented Apr 16, 2020

npx가 먹통인지 빌드는 못해봤지만 바닐라 스크립트에 대해서 구글링도 해보고 이해도 해보는 시간이 되어서 좋았습니다!! 고생하셨어요 ㅎㅎ

@NuclearPunch
Copy link
Author

npx가 먹통인지 빌드는 못해봤지만 바닐라 스크립트에 대해서 구글링도 해보고 이해도 해보는 시간이 되어서 좋았습니다!! 고생하셨어요 ㅎㅎ

이번기회에 바닐라스크립트로 해보면서 많은 반성의 시간을 갖게되었습니다 :)

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.

와.. 요거 내일 시간된다면 👠로 코드 설명 들어보고 싶어요.

어떤 관점으로 코드를 작성하셨는지 등의 개발 과정이 궁금하네요.

추가로 .gitignore에 node_modules 등이 추가되어야 할 듯 하네요. 😂(쓰레드를 읽어보니 의도하신 것인가?)

Copy link

@kand193 kand193 left a comment

Choose a reason for hiding this comment

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

바닐라 스크립트라니 진짜 대단하세요! 읽기도 좋았습니다!

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