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

feat: remove jquery (resolve #16) #41

Merged
merged 4 commits into from
Aug 2, 2019
Merged

feat: remove jquery (resolve #16) #41

merged 4 commits into from
Aug 2, 2019

Conversation

heenakwag
Copy link
Member

@heenakwag heenakwag commented Jul 30, 2019

BREAKING CHANGE: It might cause other components to fail if they pass a jQuery object as a container.

  • Remove jquery

BREAKING CHANGE: It might cause other components to fail if they pass a jQuery object as a container.
@jung-han
Copy link
Member

Can one of the admins verify this patch?

@heenakwag
Copy link
Member Author

ok to test

bower.json Show resolved Hide resolved
src/js/calendar/body.js Outdated Show resolved Hide resolved
@@ -104,25 +116,50 @@ var Header = snippet.defineClass(/** @lends Header.prototype */{
},

/**
* Set events for firing customEvents
* Set events
* @param {object} option - Constructor option
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
Member Author

Choose a reason for hiding this comment

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

_removeEvents, _setFormatters도 마찬가지인데, 이러한 의미없는 주석은 제거하는 것이 좋나요?

Copy link
Member

@seonim-ryu seonim-ryu Jul 31, 2019

Choose a reason for hiding this comment

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

팀 컨벤션으로는.. 프라이빗은 필요한 경우에만 표기하도록 정했어요ㅎ
(예전에는 필수로 작성하는 것이었으나..)

Copy link
Member

Choose a reason for hiding this comment

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

아 option을 더이상 받지 않는데 주석에 적혀있어서 해당 라인을 지우는게 어떤가 적었던건데..

Copy link
Member Author

Choose a reason for hiding this comment

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

아하 제가 라인을 잘못 봤네요 option 주석도 지울게요!

src/js/touchMouseEvent.js Outdated Show resolved Hide resolved
src/js/calendar/header.js Outdated Show resolved Hide resolved
}
};

module.exports = touchMouseEvent;
Copy link
Member

Choose a reason for hiding this comment

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

이 모듈은... tui-dom에 있으면 참 좋겠다는.. 생각이 드네요ㅎ

src/js/util.js Outdated Show resolved Hide resolved
@jung-han
Copy link
Member

리뷰 완료입니다~ 고생하셨어요

@seonim-ryu
Copy link
Member

[7/31] 리뷰 완료하였습니다. a 태그 변경하는 것 관련해서는 구두로 설명 드릴게요~

* @param {HTMLElement} elem An element
* @returns {string}
*/
getSelector: function(elem) {
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
Member Author

@heenakwag heenakwag Jul 31, 2019

Choose a reason for hiding this comment

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

로직을 변경하면서 지웠어야했는데 놓쳤네요 지우겠습니다!
datepicker/index.js_onMousedownDocument에서 사용 중 입니다.

@seonim-ryu
Copy link
Member

[7/31] 2차 리뷰 완료합니다. 넘어야될 산이 많죠?ㅎㅎ 고생 많으셨어요~

README.md Show resolved Hide resolved
Copy link
Member

@jung-han jung-han left a comment

Choose a reason for hiding this comment

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

docs에 3.0 migration guide 링크는 지워도 될 것 같아요.
4.0 마이그레이션은 jquery 밖에 없기 때문에 getting started에도 적어 주는게 좋을 것 같아요.
문서만 조금 수정해 주시면 될 것 같습니다. 리뷰 완료입니다. 고생하셨어요~

@heenakwag heenakwag merged commit 1d2b6a1 into master Aug 2, 2019
@heenakwag heenakwag deleted the feat/remove-jquery branch August 2, 2019 01:26
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