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.

Sign up for GitHub

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

Jump to bottom

fix: add custom icon error #65

Merged
merged 1 commit into from
Oct 4, 2018
Merged

fix: add custom icon error #65

merged 1 commit into from
Oct 4, 2018

Conversation

jinwoo-kim-nhn
Copy link
Contributor

@jinwoo-kim-nhn jinwoo-kim-nhn commented Oct 1, 2018 •
edited

  • 커스텀 아이콘 추가시 자바스크립트 에러가 발생되는 부분을 해결 (커스텀 아이콘은 drag로 추가되는 부분이 아니기때문에 dragevent가 생기면 안된다)
    • defaultIconPath에 포함된 타입이 아닐경우 이벤트가 발생하지 않도록 예외처리
    • 관련해서 add() 메서드 리팩토링 - (_addDragWith()) 메서드를 추가하여 코드 분리

seonim-ryu
seonim-ryu reviewed Oct 4, 2018
View reviewed changes
src/js/component/icon.js
}

resolve(this.graphics.createObjectProperties(icon));
});
}

/**
* Added icon drag
* @param {fabric.Canvas} canvas - Canvas instance
Copy link
Member

Choose a reason for hiding this comment

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

@private 추가요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 !

seonim-ryu
seonim-ryu reviewed Oct 4, 2018
View reviewed changes
src/js/component/icon.js
* Added icon drag
* @param {fabric.Canvas} canvas - Canvas instance
*/
_addWithDrag(canvas) {
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
Contributor Author

Choose a reason for hiding this comment

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

넵!

seonim-ryu
seonim-ryu reviewed Oct 4, 2018
View reviewed changes
src/js/component/icon.js
@@ -61,47 +61,55 @@ class Icon extends Component {
const canvas = this.getCanvas();
const path = this._pathMap[type];
const selectionStyle = consts.fObjectOptions.SELECTION_STYLE;
const isDefaultIconPath = Object.keys(consts.defaultIconPath).indexOf(type) >= 0;
const useDragAddIcon = this.useDragAddIcon && isDefaultIconPath;
Copy link
Member

Choose a reason for hiding this comment

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

그냥 registerdIcon은 어떨까요? 커스텀은 유저가 등록하는 아이콘이고 그 외에 것은 이미 등록되어 있으니까요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 좋은것 같습니다.

@seonim-ryu
Copy link
Member

[10/4] 리뷰 완료하였습니다~

@jinwoo-kim-nhn jinwoo-kim-nhn merged commit 692a8b0 into master Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seonim-ryu seonim-ryu seonim-ryu left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jinwoo-kim-nhn @seonim-ryu