fix: range without end #63
Conversation
test/timepicker/timepicker.spec.js
Outdated
|
|
||
| timepickerNoMeridiem.setRange(start); | ||
|
|
||
| selector = timepickerNoMeridiem.element.querySelectorAll('select[aria-label="Time"]'); |
There was a problem hiding this comment.
실제 변수에 할당되는 결과물을 생각하면 selectBoxes 정도가 변수 이름으로 적절해보이네요.
test/timepicker/timepicker.spec.js
Outdated
| selectOption = hourSelect.querySelector('option[value="8"]'); | ||
| expect(selectOption.disabled).toBe(true); | ||
| selectOption = hourSelect.querySelector('option[value="9"]'); | ||
| expect(selectOption.disabled).toBe(false); | ||
|
|
||
| timepickerNoMeridiem.setTime(9, 0); | ||
|
|
||
| selectOption = minSelect.querySelector('option[value="30"]'); | ||
| expect(selectOption.disabled).toBe(true); | ||
| selectOption = minSelect.querySelector('option[value="31"]'); | ||
| expect(selectOption.disabled).toBe(false); |
There was a problem hiding this comment.
테스트로 커버하고자 하는 범위가 더 명확했으면 좋겠습니다.
우리가 알고자 하는 것은 '(시간 선택의 경우) 8까지는 모두 disabled 이고, 9부터는 모두 enabled 이다' 입니다.
그런데 단순히 분기점 하나만 두고 이게 정상적으로 동작하는지 보장하는 것은 어폐가 있어 보이네요.
querySelectorAll 을 통해 모든 옵션을 가져온 뒤, 1 ~ 8까지 자르고, 9 ~ 12까지 자른 다음 각각의 배열이 원하는 disabled 값을 가지고 있는지 toMatchObject 단언을 활용해보면 좋겠습니다.
There was a problem hiding this comment.
hourSelect가 minSelect 둘다 같이 검증되어야 하는건가요? 만약 그게 아니라면 나누어서 검증하는게 좋을 것 같습니다. 그리고 시간이나 분을 범위 별로 검증(disabled 값)해야 한다면, 위의 리뷰처럼 범위 전체를 검증해야 정확하겠네요~! 테스트에서 클래스나 속성에 의해 자주 변경되는 요소가 아니라면 jest 스냅샷 테스트를 적용하는 것도 좋을 것 같아요.(만약 그렇지 않다면, toMatchObject를 쓰는게 좋겠구요)
test/timepicker/timepicker.spec.js
Outdated
| hourSelect = selector[0]; | ||
| minSelect = selector[1]; |
There was a problem hiding this comment.
[hourSelect, minSelect] = selector로 줄이면 좋을 것 같습니다.
src/js/timepicker/index.js
Outdated
| } else { | ||
| this.disabledMinutes[beginHour] = util.getDisabledMinuteArr([disabledMinRanges[0]]).concat(); | ||
| if (beginHour === endHour) { | ||
| this.disabledMinutes[beginHour] = util.getDisabledMinuteArr(disabledMinRanges).concat(); |
There was a problem hiding this comment.
concat()을 사용한 이유가.. 배열을 복사하기 위한 용도로 사용된 것인가요~?
There was a problem hiding this comment.
복사 용도가 맞다면 concat() 보다는 slice(0)을 사용하는 것이 좋을 것 같습니다. concat()은 배열을 연결하는 목적의 함수이기 현재 사용 의도와는 맞지 않아보여요~
There was a problem hiding this comment.
다른 코드에서 저렇게 사용하는 것을 보고 배열 복사 시 concat()을 사용하는 것으로 오해했습니다.
배열 복사를 목적으로 하는 구문에서는 slice(0)를 사용하는 것으로 수정하겠습니다!
|
리뷰 완료합니다~ |
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. 🎉 😘 ✨