fix: unable to reset selectable range #64
Conversation
test/timepicker/timepicker.spec.js
Outdated
| selectOptions = hourSelect.querySelectorAll('option'); | ||
|
|
||
| selectOptions.forEach(function(disabledHour) { | ||
| expect(disabledHour).toMatchObject({disabled: false}); |
There was a problem hiding this comment.
지난 번 리뷰 때 언급하는 것을 깜빡했는데, 일일이 forEach 로 단언은 실행하는 방법도 있고, toMatchObject 에 완성된 배열을 던져주어 배열과 배열을 통으로 비교시키는 것도 가능해요. 저는 후자가 더 괜찮다고 생각하는 편입니다.
const disabledAttrs = [...Array(selectOptions.length)].map(_ => ({disabled: false}));
expect(selectOptions).toMatchObject(disabledAttrs);There was a problem hiding this comment.
처음에는 toMatchObject에 완성된 배열을 인자로 주었었는데, 시(Hour)에는 24개, 분에는 60개의 요소를 실제로 작성을 하니 가독성이 떨어지는 느낌이 들어서(린터에 의해 한 요소 당 한 줄로 변경되어 시, 분 각각 26줄과 62줄을 단언이 차지) 반복문을 이용해 단언을 실행하는 것으로 변경했습니다.
이런 경우 전자에 비해 후자가 가지는 이점은 무엇이 있는지 궁금합니다!
There was a problem hiding this comment.
네 그래서 제가 제안한게 위의 코드처럼 아예 비교 대상인 배열을 변수로 미리 먼들어서 주는거긴 했어요.
어차피 toMatchObject 단언은 배열을 넣더라도 해당 객체의 일부만 매칭되면 되니까요.
어차피 순회를 둘 다 돌긴 하지만, 단언을 여러번 실행시키는 것 보다는 한번에 실행하고 만약에 실패하여 에러를 보여줄 때 반복문으로 실행되는 단언보다 배열끼리 비교하면 어느부분은 성공하고 어느부분은 실패했는지 스냅샷 테스트처럼 한눈에 볼 수 있게 나오는 형태가 좋겠다고 생각하기도 했고요.
There was a problem hiding this comment.
생각해보니 현재 형태에서는 몇 번째 루프에서 실패했는지 알 수 없다는 문제가 있네요!
반대로 비록 단언에 인자로 넘겨줄 배열의 형태가 가독성이 떨어지더라도 테스트 수행 시 실패한 부분을 확실히 알 수 있다는 장점이 있군요.
그럼 이번에 작성한 테스트와 이전에 작성한 테스트의 단언을 말씀해주신 방법으로 변경하도록 하겠습니다.
src/js/timepicker/index.js
Outdated
| * Reset selectable range | ||
| */ | ||
| resetRange: function() { | ||
| this.disabledHours = []; |
There was a problem hiding this comment.
옵션으로 받기도 하는 프러퍼티인데 초기화 시켜도 괜찮은 건가요?
There was a problem hiding this comment.
처음에 옵션 받을 때 처리하면 이부분은 없어도 될 것 같아요~!
There was a problem hiding this comment.
resetRange 메서드에서 this.disabledHours 프로퍼티를 초기화 하는 문을 삭제하겠습니다.
다만, 이 경우 분(Minute)에 대한 범위 설정은 초기화 되지만, 시(Hour)에 대한 범위는 초기화 되지 않기 때문에 API 명을 resetMinuteRange로 변경하도록 하겠습니다.
src/js/timepicker/index.js
Outdated
| * Reset selectable range | ||
| */ | ||
| resetRange: function() { | ||
| this.disabledHours = []; |
There was a problem hiding this comment.
처음에 옵션 받을 때 처리하면 이부분은 없어도 될 것 같아요~!
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. 🎉 😘 ✨