-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Crop preset review #84
Conversation
ok to test |
src/js/component/cropper.js
Outdated
left: -10, | ||
height: 1, | ||
width: 1 | ||
}); |
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.
defaultOption
으로 해서 객체 빼는건 어떨까요?
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.
factor
그냥.. 1이 아닌 경우에 1로 초기화 하고 this._getPresetCropSizePosition(factor)
호출하는거로 하면 될듯해요
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.
넵 반영하겠습니다.
src/js/component/cropper.js
Outdated
const standardSize = (originalWidth >= originalHeight) ? originalWidth : originalHeight; | ||
const getScale = (value, orignalValue) => (value > orignalValue) ? orignalValue / value : 1; | ||
|
||
let width = (standardSize * factor); |
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.
반영하겠습니다.
src/js/ui/crop.js
Outdated
} | ||
|
||
/** | ||
* Add event for crop | ||
* @param {Object} actions - actions for crop | ||
* @param {Function} actions.crop - crop action | ||
* @param {Function} actions.cancel - cancel action | ||
* @param {Function} actions.preset - cancel action |
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.
넵
src/js/component/cropper.js
Outdated
@@ -277,6 +278,64 @@ class Cropper extends Component { | |||
}; | |||
} | |||
|
|||
/** | |||
* Set a cropzone square | |||
* @param {number} factor - preset ratio |
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.
factor
->scale
파라미터명 변경- 파라미터가 비어있는 경우 ->
[factor]
변경
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.
네 반영했습니다.
|
||
/** | ||
* Set preset button to active status | ||
* @param {HTMLElement} button - event target element |
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.
@private
추가
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.
넵
* @param {HTMLElement} button - event target element | ||
*/ | ||
_setPresetButtonActive(button = this.defaultPresetButton) { | ||
snippet.forEach([].slice.call(this._els.preset.querySelectorAll('.preset')), presetButton => { |
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.
[].slice.call
-> 코드스니펫 메서드로 변경
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.
snippet에 따로 없고 예제에서도 따로 slice 해주라고 하네요. 이대로 하는게 좋을것 같습니다
[10/31] 리뷰 완료요~ |
[10/31] 리뷰 완료했습니다. |
</li> | ||
<li class="tui-image-editor-partition tui-image-editor-newline"> | ||
</li> | ||
<li class="tui-image-editor-partition only-left-right"> |
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.
메뉴 바 정렬이 left나 right로 되어 있을때만 보여야 하는 라인입니다.
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.
가능하면 태그 한줄로 처리하는 방법을 찾아보겠습니다.
[10/31] 리뷰 완료입니다 |
work
example