-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: customize toolbar (ref #85) #124
Conversation
f5eb6ce
to
8c09e31
Compare
*/ | ||
/* eslint-disable complexity */ | ||
static create(name, options) { | ||
let toolbarItem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static 맴버 하나뿐인 클래스인데 그냥 class로 선언하는것보다 object 리터럴로 선언하는게 직관적이지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞아요. 필요한건 사실 함수 하나뿐이긴 한데.
이것이 팩토리 패턴으로 사용된다는 것을 명확하게 하니라 통상적인 구조 그대로 유지했어요.
완료했습니다. 수고하셨습니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
작업 깔끔하게 잘했네요~ 수고하셨습니다.
eventManager.listen('stateChange', ev => { | ||
this._items.forEach(item => { | ||
if (item._state) { | ||
if (ev[item._state]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
구조 괜찮네요..
음 조금 아이디어를 더하면 state를 키로 item을 매핑하는 객체 하나를 더 하거나 toolbar에 해당 기능을 구현해서 item을 쉽게 꺼낼 수 있게 하면 더 좋을것 같아요(getItemByState()
?) 아무래도 매번 루프를 도는것 보다는 나을것 같아요. 꽤 호출되는 이벤트니까요..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다. 리팩토링은 하면 할수록 하고싶은게 계속 있네요. ㅋㅋ
_toolbar; | ||
|
||
/** | ||
* @memberof DefaultUI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요건 설명이 누락되었네요ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헷 😉
|
||
return $el; | ||
} | ||
|
||
/** | ||
* _setButton | ||
* @param {Button} button - button instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!(button instanceof Button))
조건문 체크를 봐서는 인스턴스가 아닌 경우도 있는 것 같네요~
{Button|object}
이렇게 명시해야될 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅎㅎㅎ 정신없이 리팩토링하다가 넘어간게 몇개 어렴풋이 있는데.
역시 잡아주시는군요. 감사합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[4/3] 리뷰 완료하였습니다~ 코드 깔끔하네요 👍
아 별생각없이 머지 눌러버렸네요. 리뷰 내용은 별도 커밋으로 올립니다. |
refactoring
feature
example
no deprecated entities in the diagram