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

Gyujin #1

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

Gyujin #1

wants to merge 5 commits into from

Conversation

bluejoyq
Copy link
Contributor

No description provided.

Copy link

@kimseo-0 kimseo-0 left a comment

Choose a reason for hiding this comment

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

👍👍👍 자세한 코멘트는 추후에...

Copy link

@wjdwl002 wjdwl002 left a comment

Choose a reason for hiding this comment

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

저도 바닐라 컴포넌트 개발은 처음이라 확실치는 않지만 흠없어보이네요 ! 아주 좋습니다

},
};

class App {

Choose a reason for hiding this comment

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

저는 바닐라로 컴포넌트를 만드는게 처음이라 잘 모르는데 그냥 App 클래스 하나를 만들고 내부 로직을 다 때려박으면 되나요?

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저는 이 글 보면서 개발했습니다.
https://prgms.tistory.com/53
아무래도 앱 규모가 작다 보니까 하나로 통일해서 코드를 작성했는데, 로그인 파트와 사진 가져오기 파트는 분리가 가능할 것 같네요. 수정해볼게요

Choose a reason for hiding this comment

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

하나로 하는 것보다 기능별로 스크립트를 나누는 것이 좋을 것 같습니다. 바닐라 JS 로 짜도 리액트처럼 컴포넌트 분리가 가능한걸로 알고 있습니다.
참고: https://dubaiyu.tistory.com/209

this.$photoContainer = document.getElementById('photo');
this.$loginModal = document.getElementById('modal');
this.api = api;
const $photoBtn = document.getElementById('btn-photo');

Choose a reason for hiding this comment

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

관심사의 분리를 위해 setEvent 함수를 따로 빼도 좋을 것 같네요

Copy link
Contributor Author

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.

3 participants