-
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
feat: change input bindig (#183) #306
Conversation
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.
[01/28] 리뷰 완료입니다.수고하셨습니다.
|
||
/** | ||
* Range control class | ||
* @class | ||
* @ignore | ||
*/ | ||
class Range { | ||
constructor(rangeElement, options = {}) { | ||
constructor(rangeElements, options = {}) { |
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.
JSDOC 파라미터를 표시하면 더 좋겠네요^^
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/tools/range.js
Outdated
_changeValueWithInputKeyEvent(event) { | ||
const {keyCode, target} = event; | ||
|
||
if ([38, 40].indexOf(keyCode) < 0) { |
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.
KeyCode.ArrowDown/ArrowUp 이런 식으로 정의해두고 쓰는 건 어떨까요?
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/tools/range.js
Outdated
let value = Number(target.value); | ||
|
||
if (keyCode === 38) { | ||
value += 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.
useDecimal
인 경우도 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.
미처 고려를 못했네요. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clamp로 값이 변경되지 않은 경우도 fire
하겠어요.
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/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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valueChanged
가 좀 더 어울릴것 같다는 의견 드립니다. isValueChanged
도 괜찮겠네요.
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/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.
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.
넵 모듈 안에서 한번만 인스턴스화 되도록 수정하겠습니다.
test/uiRange.spec.js
Outdated
beforeEach(() => { | ||
input = document.createElement('input'); | ||
range = new Range({ | ||
slider: document.createElement('div'), |
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.
slider도 input 생성처럼 코드 스타일을 똑같이 하는게 좋을거 같아요.
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.
넵 반영하겠습니다.
* 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
개선
다음작업