-
Notifications
You must be signed in to change notification settings - Fork 30
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
fix: consider time range on date range picker (fix #51) #84
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.
리뷰완료합니다.
src/js/dateRangePicker/index.js
Outdated
/** | ||
* Flag for time range setting in end picker | ||
* @type {boolean} | ||
* @private | ||
*/ | ||
this._isRangeSetted = 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.
외부로 나가는 프로퍼티가 아니라서 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.
내부적으로만 사용할 프로퍼티여서 @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.
이름이 isRangeSet
이 되어야 하지 않을까요?
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.
변수명을 isRangeSet
으로 변경 했으며 주석은 삭제했습니다.
|
||
it('should set minute range on endpicker', function() { | ||
var date = new Date(2021, 1, 1, 9, 30); | ||
var minuteSelect; |
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.
selectedMinute
는 어떤가요?
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.
해당 변수에 할당 되는 것이 분(minute)에 대한 옵션의 배열이어서 selectedMinute
과는 조금 다른 의미인것 같습니다.
변수명이 조금 모호한 것 같으니 명확히 minuteSelectOptions
로 변경하는 것은 어떻게 생각하시나요?
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/dateRangePicker/index.js
Outdated
* @type {number} | ||
* @private | ||
*/ | ||
this.preEndPickerDate = new Date().getDate(); |
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/dateRangePicker/index.js
Outdated
@@ -279,9 +295,62 @@ var DateRangePicker = defineClass( | |||
* // unbind the 'change:end' event | |||
* rangePicker.off('change:end'); | |||
*/ | |||
|
|||
if (this._endpicker.getDate()) { | |||
if (this.preEndPickerDate !== this._endpicker.getDate().getDate()) { |
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.
getDate()
함수가 반복 호출 되니까 어떤 역할인지 헷갈리네요.. 두 API의 역할이 구분되게끔 네이밍을 수정하는건 어렵나요?
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.
뒤에서도 사용하니 아래처럼 미리 변수로 만드는게 더 깔끔하겠네요~
const date = this._endpicker.getDate().getDate();
if (this.preEndPickerDate !== date) {
// ...
}
this.preEndPickerDate = date;
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.
미리 변수로 만들어 사용하도록 변경했습니다.
this._endPicker.getDate()
는 endPicker
에서 설정된 날짜를 Date의 인스턴스로 반환하는 메서드이고 이후 나오는 getDate()
메서드는 Date
인스턴스에서 일자를 반환하는 메서드입니다.
후자의 메서드의 경우 내장 객체의 메서드로 변경이 불가하고 전자의 메서드의 경우 DatePicker
의 공개 API이기 때문에 변경 시 사용자의 코드 변경을 요구하기 때문에 변경 하기가 어렵다고 생각했습니다.
var timeRange = {}; | ||
|
||
timeRange[startDate.getDate()] = { | ||
hour: startDate.getHours(), | ||
minute: startDate.getMinutes() | ||
}; | ||
|
||
return timeRange; |
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.
아..왜 이렇게 했나 했더니 es6문법을 못쓰는건가요..?
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.
네 현재 린트 룰이 ecma 3 버전으로 설정되어 있어서 es6 문법을 사용하지 못했습니다...
src/js/dateRangePicker/index.js
Outdated
if (pickerDate && timeRange[pickerDate.getDate()]) { | ||
endTimePicker.setRange(timeRange[pickerDate.getDate()]); | ||
this._isRangeSetted = true; | ||
} else if (this._isRangeSetted) { |
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.
이건 질문인데 범위 설정 여부(_isRangeSetted
)를 endTimePicker
로만 판단하나요? start~~
도 같이 고려해야될 것 같은데 상관없나 해서요.
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.
DateRangePicker에서 범위는 startPicker에 설정된 시각을 기준으로 endPicker에만 적용되어서 endTimePicker
에 대해 범위 설정 여부를 판단하도록 작성했습니다!
var rangedHours = [ | ||
{ disabled: true }, | ||
{ disabled: true }, | ||
{ disabled: true }, | ||
{ disabled: true }, | ||
{ disabled: true }, | ||
{ disabled: true }, | ||
{ disabled: true }, | ||
{ disabled: true }, | ||
{ disabled: true }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: false }, | ||
{ disabled: 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.
헉..ㅋㅋ 이건 반복문을 나누어서 조건별로 설정(1~9: disabled: true
, 0~24: disable: 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.
네이밍 짓기가 어렵다면 어떤 데이터인지 명확하게 주석으로라도 설명이 있으면 좋겠네요!
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.
네! 저도 만들면서 이게 맞나... 했습니다 ㅎㅎㅎ
toMatchObject
에 인자로 전달되는 배열의 형태는 시, 분이 동일하니 범용적으로 사용할 수 있도록 배열을 생성해서 반환하는 함수를 하나 만들도록 하겠습니다.
변수명도 좀 더 명확하게 변경하고 계속 모호하다면 주석을 작성하는 것도 고려하도록 하겠습니다.
container: startpickerContainer | ||
}, | ||
endpicker: { | ||
date: new Date(), |
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.
이후에 endPicker
의 일자를 설정하는 문이 있어서 테스트에 영향은 없습니다.
그러나 해당 옵션은 테스트에 불필요한 옵션이기 때문에 삭제했습니다.
@@ -106,6 +196,56 @@ describe('DateRangePicker', function() { | |||
expect(picker.getStartDate()).toEqual(new Date(2018, 0, 1)); | |||
}); | |||
|
|||
it('should set hour range on endpicker', function() { |
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.
테스트 디스크립션이 더 명확하면 좋겠어요~ㅎㅎ 범위 설정에 따라서 옵션 필드들의 disabled
값이 바뀌는데 이게 정확히 어떻게 바뀌어야 하는지 자세히 드러나면 좋겠습니다! 아래도 마찬가지입니다.
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.
리뷰 완료입니다!
chore: add function for getting array that is expected chore: rename variables to match convention chore: rename variables appropriately chore: remove unnecessary jsdoc chore: change the test description to clarify
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/dateRangePicker/index.js
Outdated
var date; | ||
|
||
if (this._endpicker.getDate()) { | ||
date = this._endpicker.getDate().getDate(); |
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.
코드베이스에 익숙하지 않은 입장에서 getDate
가 2중첩으로 호출되니 이게 뭐에 쓰이는건가 좀 의아하긴 하네요.
아마 각 픽커에에서 getDate
를 호출하면 Date
인스턴스가 나오고 거기다가 Date#getDate
를 호출하는 느낌 같긴 한데.. 그렇게 된다면 뜻이 구별되면 더 좋겠다는 생각이 듭니다.
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.
this._endpicker.getDate()
가 조건문에서도 사용되어서 변수에 담아 사용하는 것으로 변경해 getDate().getDate()
로 호출되는 것을 수정했습니다.
이렇게 하면 의아함이 조금은 나아지는 것 같습니다.
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.
리뷰 완료합니다.
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.
리뷰 완료합니다.
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.
리뷰 완료합니다.
Please check if the PR fulfills these requirements
fix #xxx[,#xxx]
, where "xxx" is the issue number)Description
Thank you for your contribution to TOAST UI product. 🎉 😘 ✨