feat: change input bindig (#183) #306
Conversation
jinwoo-kim-nhn
changed the title
jinwoo-kim-nhn
changed the title
|
|
||
| /** | ||
| * Range control class | ||
| * @class | ||
| * @ignore | ||
| */ | ||
| class Range { | ||
| constructor(rangeElement, options = {}) { | ||
| constructor(rangeElements, options = {}) { |
There was a problem hiding this comment.
JSDOC 파라미터를 표시하면 더 좋겠네요^^
src/js/ui/tools/range.js
Outdated
| _changeValueWithInputKeyEvent(event) { | ||
| const {keyCode, target} = event; | ||
|
|
||
| if ([38, 40].indexOf(keyCode) < 0) { |
There was a problem hiding this comment.
KeyCode.ArrowDown/ArrowUp 이런 식으로 정의해두고 쓰는 건 어떨까요?
There was a problem hiding this comment.
네 그렇네요 ㅎㅎ 수정하였습니다.
src/js/ui/tools/range.js
Outdated
| let value = Number(target.value); | ||
|
|
||
| if (keyCode === 38) { | ||
| value += 1; |
There was a problem hiding this comment.
useDecimal인 경우도 1씩 증감하게 되나요? 소수점은 처리하지 않아도 되는건지 문의 드립니다.
There was a problem hiding this comment.
미처 고려를 못했네요. useDecimal 인 경우는 0.1단위로 증감되도록 수정하겠습니다.
src/js/ui/tools/range.js
Outdated
| value = clamp(value, this._min, this.max); | ||
|
|
||
| this.value = value; | ||
| this.fire('change', value, false); |
There was a problem hiding this comment.
clamp로 값이 변경되지 않은 경우도 fire하겠어요.
There was a problem hiding this comment.
네.. 그렇네요 수정하겠습니다.
src/js/ui/tools/range.js
Outdated
| const value = (this._absMax * ratio) + this._min; | ||
| const resultValue = (this._absMax * ratio) + this._min; | ||
| const value = this._useDecimal ? resultValue : toInteger(resultValue); | ||
| const changedValue = this.value !== value; |
There was a problem hiding this comment.
valueChanged가 좀 더 어울릴것 같다는 의견 드립니다. isValueChanged도 괜찮겠네요.
src/js/ui/tools/range.js
Outdated
| * @private | ||
| */ | ||
| _filterForInputText(inputValue) { | ||
| return inputValue.replace(/(-?)([0-9]*)[^0-9]*([0-9]*)/g, '$1$2$3'); |
There was a problem hiding this comment.
상하 키보드 이벤트가 올 때마다 정규식 인스턴스가 생성되는데요, 이 경우는 키 이벤트가 많이 오는건 아니라서 성능상 별로 다른 건 없겠지만 빈번한 호출인 경우는 불필요한 인스턴스 생성이 많아집니다. 가능하다면 모듈에서 한번만 인스턴스화 하고 쓰면 어떨까 합니다.
There was a problem hiding this comment.
넵 모듈 안에서 한번만 인스턴스화 되도록 수정하겠습니다.
test/uiRange.spec.js
Outdated
| beforeEach(() => { | ||
| input = document.createElement('input'); | ||
| range = new Range({ | ||
| slider: document.createElement('div'), |
There was a problem hiding this comment.
slider도 input 생성처럼 코드 스타일을 똑같이 하는게 좋을거 같아요.
* middle commit * feat: apply input ui in range component * feat: appy value with change input * feat: some refactoring (change naming, added jsdoc) * fix: added jsdoc for missing jsdoc * feat: some refactoring for remove event binding * apply code review
Please check if the PR fulfills these requirements
fix #xxx[,#xxx], where "xxx" is the issue number)Description
issue
개선
다음작업