-
Notifications
You must be signed in to change notification settings - Fork 12
fix: set wrong time when setting time range if use time step (fix #65) #67
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
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/timepicker/index.js
Outdated
@@ -768,8 +768,16 @@ var TimePicker = defineClass( | |||
* @private | |||
*/ | |||
applyRange: function(beginHour, beginMin, endHour) { | |||
var toSetHour = beginHour; |
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.
변수명이 약간 어색하네요. 그냥 hour
나 minute
로 해도 괜찮을 것 같은데 이건 단순 아이디어이니 반영 안하셔도 돼요~
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.
저도 이런 상황에서는 target
이라는 접두사를 더 많이 사용해서 그런지 어색하네요.
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.
접두사를 toSet
에서 target
으로 변경했습니다.
단순히 hour
와 minute
로 사용하니 정확히 어디에 쓰이는 변수인지 알기 조금 어려운 느낌이 있어 제시해주신 접두사를 붙이는 것으로 했습니다.
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.
리뷰 완료합니다.
맥락이 조금 부족해서 그런데 반영되는 부분에 대한 구현 의도가 PR Description에 적혀있으면 더 꼼꼼히 리뷰를 진행할 수 있겠어요.
src/js/timepicker/index.js
Outdated
@@ -768,8 +768,16 @@ var TimePicker = defineClass( | |||
* @private | |||
*/ | |||
applyRange: function(beginHour, beginMin, endHour) { | |||
var toSetHour = beginHour; |
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.
저도 이런 상황에서는 target
이라는 접두사를 더 많이 사용해서 그런지 어색하네요.
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. 🎉 😘 ✨