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.

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

Merged
merged 7 commits into from
Jan 28, 2020
Merged

Conversation

jinwoo-kim-nhn
Copy link
Contributor

@jinwoo-kim-nhn jinwoo-kim-nhn commented Jan 21, 2020

Please check if the PR fulfills these requirements

  • It's submitted to right branch according to our branching model
  • It's right issue type on title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has description for the breaking change

Description

issue

개선

  • tools/range.js 에서 input 영역의 값 표시와 변경에 관련된 이벤트를 직접 처리할수 있도록 역할 강화
  • tools/range.js 사용시 소수점 단위로 정교하게 다뤄야 하는 경우와 정수단위로 값을 다뤄야 하는 경우가 있으므로 소수점값 변경을 range 컴포넌트가 직접 고려하도록 useDecimal 옵션 추가

다음작업

  • range 컴포넌트로 값 변경시 변경과정에서 undo, redo stack 반영 및 되돌리기가 매끄럽게 동작하지 않는 부분은 해당 개선 이슈와는 관련이 없는 기존 버그로.. 다음 차례의 이슈로 바로 이어서 작업 예정.

@jinwoo-kim-nhn jinwoo-kim-nhn changed the title Feat/change input bindig feat: change input bindig (fixed #183) Jan 23, 2020
@jinwoo-kim-nhn jinwoo-kim-nhn changed the title feat: change input bindig (fixed #183) feat: change input bindig (#183) Jan 23, 2020
Copy link
Contributor

@dongsik-yoo dongsik-yoo left a 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 = {}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

JSDOC 파라미터를 표시하면 더 좋겠네요^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 수정하였습니다.

_changeValueWithInputKeyEvent(event) {
const {keyCode, target} = event;

if ([38, 40].indexOf(keyCode) < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

KeyCode.ArrowDown/ArrowUp 이런 식으로 정의해두고 쓰는 건 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네 그렇네요 ㅎㅎ 수정하였습니다.

let value = Number(target.value);

if (keyCode === 38) {
value += 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

useDecimal인 경우도 1씩 증감하게 되나요? 소수점은 처리하지 않아도 되는건지 문의 드립니다.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

미처 고려를 못했네요. useDecimal 인 경우는 0.1단위로 증감되도록 수정하겠습니다.

value = clamp(value, this._min, this.max);

this.value = value;
this.fire('change', value, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

clamp로 값이 변경되지 않은 경우도 fire하겠어요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네.. 그렇네요 수정하겠습니다.

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;
Copy link
Contributor

Choose a reason for hiding this comment

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

valueChanged가 좀 더 어울릴것 같다는 의견 드립니다. isValueChanged도 괜찮겠네요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 반영하겠습니다.

* @private
*/
_filterForInputText(inputValue) {
return inputValue.replace(/(-?)([0-9]*)[^0-9]*([0-9]*)/g, '$1$2$3');
Copy link
Contributor

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.

넵 모듈 안에서 한번만 인스턴스화 되도록 수정하겠습니다.

beforeEach(() => {
input = document.createElement('input');
range = new Range({
slider: document.createElement('div'),
Copy link
Contributor

Choose a reason for hiding this comment

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

slider도 input 생성처럼 코드 스타일을 똑같이 하는게 좋을거 같아요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

넵 반영하겠습니다.

@jinwoo-kim-nhn jinwoo-kim-nhn merged commit b1cec99 into master Jan 28, 2020
@jinwoo-kim-nhn jinwoo-kim-nhn deleted the feat/changeInputBindig branch March 20, 2020 08:13
HerlinMatos pushed a commit to EveryMundo/tui.image-editor that referenced this pull request Jul 2, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants