feat: undo redo for slider filter (fixed #310) #319
Conversation
jinwoo-kim-nhn
changed the title
src/js/action.js
Outdated
| @@ -41,8 +41,14 @@ export default { | |||
| this.ui.rotate.setRangeBarAngle('setAngle', angle); | |||
| } | |||
| }; | |||
| const setFilterSateRangeBarOnAction = filterOptions => { | |||
There was a problem hiding this comment.
오타 수정하겠습니다. 감사합니다.
src/js/command/applyFilter.js
Outdated
| const maskObj = graphics.getObject(maskObjId); | ||
|
|
||
| if (!(maskObj && maskObj.isType('image'))) { | ||
| return Promise.reject(rejectMessages.invalidParameters); |
There was a problem hiding this comment.
이 함수가 Promise.reject을 하는 것은 안 맞는 것 같아요.
There was a problem hiding this comment.
네 맞네요. 수정하겠습니다
src/js/command/applyFilter.js
Outdated
| graphics.remove(options.mask); | ||
| } else { | ||
| this.undoData.options = filterComp.getOptions(type); | ||
| if (!isSilent) { |
There was a problem hiding this comment.
cacheUndoDataForSilent가 꼭 필요할까 싶습니다. isSilent가 아니면 undoData를 만들고, isSilent이면 최초 한번만 만들면 어떨까요? undoData는 Command의 첫 실행의 상태값만 보관하면 되기 때문입니다.
There was a problem hiding this comment.
isSilent === true 이면 처음에만 undoData를 만들도록 수정하겠습니다.
There was a problem hiding this comment.
다시 생각해보니 서로 다른 명령간에 undoData가 섞이면 안되기 때문에, 각각의 명령별로 cacheUndo가 필요하네요 전체적으로 다시 수정하였습니다.
| @@ -77,50 +77,122 @@ class Filter extends Submenu { | |||
| * @param {Function} actions.applyFilter - apply filter option | |||
| */ | |||
| addEvent({applyFilter}) { | |||
| const changeRangeValue = this._changeRangeValue.bind(this, applyFilter); | |||
| const changeFilterState = filterName => this._changeFilterState.bind(this, applyFilter, filterName); | |||
There was a problem hiding this comment.
addEvent에서 바인딩한 이벤트를 해제할 필요는 없나요?
There was a problem hiding this comment.
넵. detroy 만드는걸 빼먹었네요 반영하겠습니다.
There was a problem hiding this comment.
이 부분은 일단 작업해 놨습니다만, ImageEditor.destroy 할때 UI 부분을 전체적으로 destroy 하는 부분이 안되고 있네요. 바로 이슈 잡아서 이후 작업으로 나머지도 진행 하겠습니다.
| @@ -41,8 +41,14 @@ export default { | |||
| this.ui.rotate.setRangeBarAngle('setAngle', angle); | |||
| } | |||
| }; | |||
| const setFilterSateRangeBarOnAction = filterOptions => { | |||
| if (this.ui.submenu === 'filter') { | |||
| this.ui.filter.setFilterState(filterOptions); | |||
There was a problem hiding this comment.
한 파일에 this.ui가 꽤 많이 나오는데 캐싱해서 쓰면 어떨까요 ㅎㅎ
There was a problem hiding this comment.
캐싱해서 쓰면 참조가 깨지면서 에러가 나는 현상이 있네요;; 이부분은 이 pr에서는 일단 두고 추후에 신중이 일괄 작업하겠습니다.
src/js/command/applyFilter.js
Outdated
| undoData.options = prevfilterOption; | ||
| } else { | ||
| undoData.options = cacheUndoData.options; | ||
| } |
There was a problem hiding this comment.
여기 삼항 연산자를 쓰면 어떨까요??
undoData.options = cacheUndoData ? cacheUndoData.options : prevfilterOption;There was a problem hiding this comment.
네 이 부분 반영하겠습니다.
* feat: slider undo redo for filter - middle commit * middle commit * feat: prototype complete * fix: added test and refactoring * refactor: add jsdoc with filter typename map * refactor: isSilent for cacheUndodata * apply codereview Co-authored-by: superlucky84 <[email protected]>
Please check if the PR fulfills these requirements
fix #xxx[,#xxx], where "xxx" is the issue number)Description
issue
undoStackwhile dragging in the slider UI. #310슬라이더 UI의 drag도중에는 undo 스택에 반영 안되도록 개선
불필요한 sepia2: 'vintage' mapping을 제거 하고 vintage: 'vintage' 만 남김